]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: prevent mutual deadlock between GC stopTheWorld and suspendG
authorMichael Anthony Knyszek <mknyszek@google.com>
Sat, 14 Jun 2025 02:45:08 +0000 (02:45 +0000)
committerGopher Robot <gobot@golang.org>
Wed, 18 Jun 2025 18:23:10 +0000 (11:23 -0700)
Almost everywhere we stop the world we casGToWaitingForGC to prevent
mutual deadlock with the GC trying to scan our stack. This historically
was only necessary if we weren't stopping the world to change the GC
phase, because what we were worried about was mutual deadlock with mark
workers' use of suspendG. And, they were the only users of suspendG.

In Go 1.22 this changed. The execution tracer began using suspendG, too.
This leads to the possibility of mutual deadlock between the execution
tracer and a goroutine trying to start or end the GC mark phase. The fix
is simple: make the stop-the-world calls for the GC also call
casGToWaitingForGC. This way, suspendG is guaranteed to make progress in
this circumstance, and once it completes, the stop-the-world can
complete as well.

We can take this a step further, though, and move casGToWaitingForGC
into stopTheWorldWithSema, since there's no longer really a place we can
afford to skip this detail.

While we're here, rename casGToWaitingForGC to casGToWaitingForSuspendG,
since the GC is now not the only potential source of mutual deadlock.

Fixes #72740.

Change-Id: I5e3739a463ef3e8173ad33c531e696e46260692f
Reviewed-on: https://go-review.googlesource.com/c/go/+/681501
Reviewed-by: Carlos Amedee <carlos@golang.org>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
src/runtime/mgc.go
src/runtime/mgcmark.go
src/runtime/proc.go
src/runtime/runtime2.go
src/runtime/stack.go
src/runtime/trace.go
src/runtime/tracestatus.go

index 38f343164cce3dc1efbf7f57c39163dca199a205..f2df1a00e0c6839d974f80ff03cace72ad2dab1e 100644 (file)
@@ -1048,7 +1048,7 @@ func gcMarkTermination(stw worldStop) {
        // N.B. The execution tracer is not aware of this status
        // transition and handles it specially based on the
        // wait reason.
-       casGToWaitingForGC(curgp, _Grunning, waitReasonGarbageCollection)
+       casGToWaitingForSuspendG(curgp, _Grunning, waitReasonGarbageCollection)
 
        // Run gc on the g0 stack. We do this so that the g stack
        // we're currently running on will no longer change. Cuts
@@ -1522,7 +1522,8 @@ func gcBgMarkWorker(ready chan struct{}) {
 
                systemstack(func() {
                        // Mark our goroutine preemptible so its stack
-                       // can be scanned. This lets two mark workers
+                       // can be scanned or observed by the execution
+                       // tracer. This, for example, lets two mark workers
                        // scan each other (otherwise, they would
                        // deadlock). We must not modify anything on
                        // the G stack. However, stack shrinking is
@@ -1532,7 +1533,7 @@ func gcBgMarkWorker(ready chan struct{}) {
                        // N.B. The execution tracer is not aware of this status
                        // transition and handles it specially based on the
                        // wait reason.
-                       casGToWaitingForGC(gp, _Grunning, waitReasonGCWorkerActive)
+                       casGToWaitingForSuspendG(gp, _Grunning, waitReasonGCWorkerActive)
                        switch pp.gcMarkWorkerMode {
                        default:
                                throw("gcBgMarkWorker: unexpected gcMarkWorkerMode")
index 507aac748282f14d28f7ec6d37c720ad859f25c6..a136c7aeaceda245e37cbe2eee09f1b3e413fb43 100644 (file)
@@ -227,7 +227,7 @@ func markroot(gcw *gcWork, i uint32, flushBgCredit bool) int64 {
                        userG := getg().m.curg
                        selfScan := gp == userG && readgstatus(userG) == _Grunning
                        if selfScan {
-                               casGToWaitingForGC(userG, _Grunning, waitReasonGarbageCollectionScan)
+                               casGToWaitingForSuspendG(userG, _Grunning, waitReasonGarbageCollectionScan)
                        }
 
                        // TODO: suspendG blocks (and spins) until gp
@@ -682,7 +682,7 @@ func gcAssistAlloc1(gp *g, scanWork int64) {
        }
 
        // gcDrainN requires the caller to be preemptible.
-       casGToWaitingForGC(gp, _Grunning, waitReasonGCAssistMarking)
+       casGToWaitingForSuspendG(gp, _Grunning, waitReasonGCAssistMarking)
 
        // drain own cached work first in the hopes that it
        // will be more cache friendly.
index 37a7b7f6849e7d8d1ec6faf76836707e617e8911..98173084302a33a32e5a1946dff1bf83ae16d487 100644 (file)
@@ -1347,13 +1347,13 @@ func casGToWaiting(gp *g, old uint32, reason waitReason) {
        casgstatus(gp, old, _Gwaiting)
 }
 
-// casGToWaitingForGC transitions gp from old to _Gwaiting, and sets the wait reason.
-// The wait reason must be a valid isWaitingForGC wait reason.
+// casGToWaitingForSuspendG transitions gp from old to _Gwaiting, and sets the wait reason.
+// The wait reason must be a valid isWaitingForSuspendG wait reason.
 //
 // Use this over casgstatus when possible to ensure that a waitreason is set.
-func casGToWaitingForGC(gp *g, old uint32, reason waitReason) {
-       if !reason.isWaitingForGC() {
-               throw("casGToWaitingForGC with non-isWaitingForGC wait reason")
+func casGToWaitingForSuspendG(gp *g, old uint32, reason waitReason) {
+       if !reason.isWaitingForSuspendG() {
+               throw("casGToWaitingForSuspendG with non-isWaitingForSuspendG wait reason")
        }
        casGToWaiting(gp, old, reason)
 }
@@ -1487,23 +1487,7 @@ func stopTheWorld(reason stwReason) worldStop {
        gp := getg()
        gp.m.preemptoff = reason.String()
        systemstack(func() {
-               // Mark the goroutine which called stopTheWorld preemptible so its
-               // stack may be scanned.
-               // This lets a mark worker scan us while we try to stop the world
-               // since otherwise we could get in a mutual preemption deadlock.
-               // We must not modify anything on the G stack because a stack shrink
-               // may occur. A stack shrink is otherwise OK though because in order
-               // to return from this function (and to leave the system stack) we
-               // must have preempted all goroutines, including any attempting
-               // to scan our stack, in which case, any stack shrinking will
-               // have already completed by the time we exit.
-               //
-               // N.B. The execution tracer is not aware of this status
-               // transition and handles it specially based on the
-               // wait reason.
-               casGToWaitingForGC(gp, _Grunning, waitReasonStoppingTheWorld)
                stopTheWorldContext = stopTheWorldWithSema(reason) // avoid write to stack
-               casgstatus(gp, _Gwaiting, _Grunning)
        })
        return stopTheWorldContext
 }
@@ -1592,7 +1576,30 @@ var gcsema uint32 = 1
 //
 // Returns the STW context. When starting the world, this context must be
 // passed to startTheWorldWithSema.
+//
+//go:systemstack
 func stopTheWorldWithSema(reason stwReason) worldStop {
+       // Mark the goroutine which called stopTheWorld preemptible so its
+       // stack may be scanned by the GC or observed by the execution tracer.
+       //
+       // This lets a mark worker scan us or the execution tracer take our
+       // stack while we try to stop the world since otherwise we could get
+       // in a mutual preemption deadlock.
+       //
+       // We must not modify anything on the G stack because a stack shrink
+       // may occur, now that we switched to _Gwaiting, specifically if we're
+       // doing this during the mark phase (mark termination excepted, since
+       // we know that stack scanning is done by that point). A stack shrink
+       // is otherwise OK though because in order to return from this function
+       // (and to leave the system stack) we must have preempted all
+       // goroutines, including any attempting to scan our stack, in which
+       // case, any stack shrinking will have already completed by the time we
+       // exit.
+       //
+       // N.B. The execution tracer is not aware of this status transition and
+       // andles it specially based on the wait reason.
+       casGToWaitingForSuspendG(getg().m.curg, _Grunning, waitReasonStoppingTheWorld)
+
        trace := traceAcquire()
        if trace.ok() {
                trace.STWStart(reason)
@@ -1700,6 +1707,9 @@ func stopTheWorldWithSema(reason stwReason) worldStop {
 
        worldStopped()
 
+       // Switch back to _Grunning, now that the world is stopped.
+       casgstatus(getg().m.curg, _Gwaiting, _Grunning)
+
        return worldStop{
                reason:           reason,
                startedStopping:  start,
@@ -2068,15 +2078,23 @@ found:
 func forEachP(reason waitReason, fn func(*p)) {
        systemstack(func() {
                gp := getg().m.curg
-               // Mark the user stack as preemptible so that it may be scanned.
-               // Otherwise, our attempt to force all P's to a safepoint could
-               // result in a deadlock as we attempt to preempt a worker that's
-               // trying to preempt us (e.g. for a stack scan).
+               // Mark the user stack as preemptible so that it may be scanned
+               // by the GC or observed by the execution tracer. Otherwise, our
+               // attempt to force all P's to a safepoint could result in a
+               // deadlock as we attempt to preempt a goroutine that's trying
+               // to preempt us (e.g. for a stack scan).
+               //
+               // We must not modify anything on the G stack because a stack shrink
+               // may occur. A stack shrink is otherwise OK though because in order
+               // to return from this function (and to leave the system stack) we
+               // must have preempted all goroutines, including any attempting
+               // to scan our stack, in which case, any stack shrinking will
+               // have already completed by the time we exit.
                //
                // N.B. The execution tracer is not aware of this status
                // transition and handles it specially based on the
                // wait reason.
-               casGToWaitingForGC(gp, _Grunning, reason)
+               casGToWaitingForSuspendG(gp, _Grunning, reason)
                forEachPInternal(fn)
                casgstatus(gp, _Gwaiting, _Grunning)
        })
index d1b31be172e65f586fdf4995fbd3a9bbbb915522..96720846b24559e544e2149e98102334e907328d 100644 (file)
@@ -1163,17 +1163,17 @@ func (w waitReason) isMutexWait() bool {
                w == waitReasonSyncRWMutexLock
 }
 
-func (w waitReason) isWaitingForGC() bool {
-       return isWaitingForGC[w]
+func (w waitReason) isWaitingForSuspendG() bool {
+       return isWaitingForSuspendG[w]
 }
 
-// isWaitingForGC indicates that a goroutine is only entering _Gwaiting and
-// setting a waitReason because it needs to be able to let the GC take ownership
-// of its stack. The G is always actually executing on the system stack, in
-// these cases.
+// isWaitingForSuspendG indicates that a goroutine is only entering _Gwaiting and
+// setting a waitReason because it needs to be able to let the suspendG
+// (used by the GC and the execution tracer) take ownership of its stack.
+// The G is always actually executing on the system stack in these cases.
 //
 // TODO(mknyszek): Consider replacing this with a new dedicated G status.
-var isWaitingForGC = [len(waitReasonStrings)]bool{
+var isWaitingForSuspendG = [len(waitReasonStrings)]bool{
        waitReasonStoppingTheWorld:      true,
        waitReasonGCMarkTermination:     true,
        waitReasonGarbageCollection:     true,
index 7e69d65fbb7e501920399e6bd8db841d2805cb29..4b647976f075b18e7933ab2348bb91aea2173d57 100644 (file)
@@ -1212,14 +1212,14 @@ func isShrinkStackSafe(gp *g) bool {
                return false
        }
        // We also can't copy the stack while tracing is enabled, and
-       // gp is in _Gwaiting solely to make itself available to the GC.
+       // gp is in _Gwaiting solely to make itself available to suspendG.
        // In these cases, the G is actually executing on the system
        // stack, and the execution tracer may want to take a stack trace
        // of the G's stack. Note: it's safe to access gp.waitreason here.
        // We're only checking if this is true if we took ownership of the
        // G with the _Gscan bit. This prevents the goroutine from transitioning,
        // which prevents gp.waitreason from changing.
-       if traceEnabled() && readgstatus(gp)&^_Gscan == _Gwaiting && gp.waitreason.isWaitingForGC() {
+       if traceEnabled() && readgstatus(gp)&^_Gscan == _Gwaiting && gp.waitreason.isWaitingForSuspendG() {
                return false
        }
        return true
index 871871c2795a84fecd659f1d3162c5ff0ca7505e..b92e7b4e8e36569b89deae70c77b004da6853733 100644 (file)
@@ -376,7 +376,7 @@ func traceAdvance(stopTrace bool) {
                        me := getg().m.curg
                        // We don't have to handle this G status transition because we
                        // already eliminated ourselves from consideration above.
-                       casGToWaitingForGC(me, _Grunning, waitReasonTraceGoroutineStatus)
+                       casGToWaitingForSuspendG(me, _Grunning, waitReasonTraceGoroutineStatus)
                        // We need to suspend and take ownership of the G to safely read its
                        // goid. Note that we can't actually emit the event at this point
                        // because we might stop the G in a window where it's unsafe to write
index 4dabc8e562f3de41d17c0ef6bdd87e0e740e29e9..03ec81fc0262a1f496c471884321a401aaa0a256 100644 (file)
@@ -126,11 +126,12 @@ func goStatusToTraceGoStatus(status uint32, wr waitReason) tracev2.GoStatus {
                // There are a number of cases where a G might end up in
                // _Gwaiting but it's actually running in a non-preemptive
                // state but needs to present itself as preempted to the
-               // garbage collector. In these cases, we're not going to
-               // emit an event, and we want these goroutines to appear in
-               // the final trace as if they're running, not blocked.
+               // garbage collector and traceAdvance (via suspendG). In
+               // these cases, we're not going to emit an event, and we
+               // want these goroutines to appear in the final trace as
+               // if they're running, not blocked.
                tgs = tracev2.GoWaiting
-               if status == _Gwaiting && wr.isWaitingForGC() {
+               if status == _Gwaiting && wr.isWaitingForSuspendG() {
                        tgs = tracev2.GoRunning
                }
        case _Gdead: