From cea35baf12cd8996c97f6dd3e62a0165d0e50843 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Tue, 21 Nov 2023 23:57:36 +0000 Subject: [PATCH] runtime: hold sched.lock over traceThreadDestroy in dropm This is required by traceThreadDestroy, though it's not strictly necessary in this case. The requirement to hold sched.lock comes from the assumption that traceThreadDestroy is getting called when the thread leaves the tracer's view, but in this case the extra m that dropm is dropping never leaves the allm list. Nevertheless, traceThreadDestroy requires it just as a safety measure, and that's reasonable. dropm is generally rare on pthread platforms, so the extra lock acquire over this short critical section (and only when tracing is enabled) is fine. Change-Id: Ib631820963c74f2f087d14a0067d0441d75d6785 Reviewed-on: https://go-review.googlesource.com/c/go/+/544396 Reviewed-by: Matthew Dempsky Run-TryBot: Michael Knyszek Reviewed-by: Cherry Mui TryBot-Result: Gopher Robot --- src/runtime/proc.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/runtime/proc.go b/src/runtime/proc.go index e760572906..7a2283e360 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -2428,8 +2428,20 @@ func dropm() { // Flush all the M's buffers. This is necessary because the M might // be used on a different thread with a different procid, so we have // to make sure we don't write into the same buffer. - if traceEnabled() || traceShuttingDown() { + // + // N.B. traceThreadDestroy is a no-op in the old tracer, so avoid the + // unnecessary acquire/release of the lock. + if goexperiment.ExecTracer2 && (traceEnabled() || traceShuttingDown()) { + // Acquire sched.lock across thread destruction. One of the invariants of the tracer + // is that a thread cannot disappear from the tracer's view (allm or freem) without + // it noticing, so it requires that sched.lock be held over traceThreadDestroy. + // + // This isn't strictly necessary in this case, because this thread never leaves allm, + // but the critical section is short and dropm is rare on pthread platforms, so just + // take the lock and play it safe. traceThreadDestroy also asserts that the lock is held. + lock(&sched.lock) traceThreadDestroy(mp) + unlock(&sched.lock) } mp.isExtraInSig = false -- 2.48.1