From da6ddd1df8ef9684d548ec17e7f0e51f5427211b Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Mon, 29 Mar 2021 10:50:40 -0700
Subject: [PATCH] [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
---
 pkg/sentry/platform/ptrace/subprocess.go | 58 ++++++++++++++----------
 1 file changed, 35 insertions(+), 23 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
-			}
-		}
-
-		// Create a new thread.
-		t = newThread()
-		tp.threads[currentTID] = t
+	tp.mu.RUnlock()
+	if ok {
+		return t
 	}
-	tp.mu.Unlock()
+
+	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
+		}
+	}
+
+	// Create a new thread.
+	t = newThread()
+	tp.threads[currentTID] = t
 	return t
 }