summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorAyush Ranjan <ayushranjan@google.com>2021-03-29 10:50:40 -0700
committergVisor bot <gvisor-bot@google.com>2021-03-29 10:52:19 -0700
commitda6ddd1df8ef9684d548ec17e7f0e51f5427211b (patch)
treed7f47b5468c6b19e9378c2d4c4e4b6abae692979
parentfbec65fc3f21773cbec3db4aadf27b85e8859448 (diff)
[perf] Reduce contention in ptrace.threadPool.lookupOrCreate().
lookupOrCreate is called from subprocess.switchToApp() and subprocess.syscall(). lookupOrCreate() looks for a thread already created for the current TID. If a thread exists (common case), it returns immediately. Otherwise it creates a new one. This change switches to using a sync.RWMutex. The initial thread existence lookup is now done only with the read lock. So multiple successful lookups can occur concurrently. Only when a new thread is created will it acquire the lock for writing and update the map (which is not the common case). Discovered in mutex profiles from the various ptrace benchmarks. Example: https://gvisor.dev/profile/gvisor-buildkite/fd14bfad-b30f-44dc-859b-80ebac50beb4/843827db-da50-4dc9-a2ea-ecf734dde2d5/tmp/profile/ptrace/BenchmarkFio/operation.write/blockSize.4K/filesystem.tmpfs/benchmarks/fio/mutex.pprof/flamegraph PiperOrigin-RevId: 365612094
-rw-r--r--pkg/sentry/platform/ptrace/subprocess.go56
1 files changed, 34 insertions, 22 deletions
diff --git a/pkg/sentry/platform/ptrace/subprocess.go b/pkg/sentry/platform/ptrace/subprocess.go
index acccbfe2e..d2284487a 100644
--- a/pkg/sentry/platform/ptrace/subprocess.go
+++ b/pkg/sentry/platform/ptrace/subprocess.go
@@ -69,7 +69,7 @@ type thread struct {
// threadPool is a collection of threads.
type threadPool struct {
// mu protects below.
- mu sync.Mutex
+ mu sync.RWMutex
// threads is the collection of threads.
//
@@ -85,30 +85,42 @@ type threadPool struct {
//
// Precondition: the runtime OS thread must be locked.
func (tp *threadPool) lookupOrCreate(currentTID int32, newThread func() *thread) *thread {
- tp.mu.Lock()
+ // The overwhelming common case is that the thread is already created.
+ // Optimistically attempt the lookup by only locking for reading.
+ tp.mu.RLock()
t, ok := tp.threads[currentTID]
- if !ok {
- // Before creating a new thread, see if we can find a thread
- // whose system tid has disappeared.
- //
- // TODO(b/77216482): Other parts of this package depend on
- // threads never exiting.
- for origTID, t := range tp.threads {
- // Signal zero is an easy existence check.
- if err := unix.Tgkill(unix.Getpid(), int(origTID), 0); err != nil {
- // This thread has been abandoned; reuse it.
- delete(tp.threads, origTID)
- tp.threads[currentTID] = t
- tp.mu.Unlock()
- return t
- }
- }
+ tp.mu.RUnlock()
+ if ok {
+ return t
+ }
- // Create a new thread.
- t = newThread()
- tp.threads[currentTID] = t
+ tp.mu.Lock()
+ defer tp.mu.Unlock()
+
+ // Another goroutine might have created the thread for currentTID in between
+ // mu.RUnlock() and mu.Lock().
+ if t, ok = tp.threads[currentTID]; ok {
+ return t
+ }
+
+ // Before creating a new thread, see if we can find a thread
+ // whose system tid has disappeared.
+ //
+ // TODO(b/77216482): Other parts of this package depend on
+ // threads never exiting.
+ for origTID, t := range tp.threads {
+ // Signal zero is an easy existence check.
+ if err := unix.Tgkill(unix.Getpid(), int(origTID), 0); err != nil {
+ // This thread has been abandoned; reuse it.
+ delete(tp.threads, origTID)
+ tp.threads[currentTID] = t
+ return t
+ }
}
- tp.mu.Unlock()
+
+ // Create a new thread.
+ t = newThread()
+ tp.threads[currentTID] = t
return t
}