runtime: prevent calls to GOMAXPROCS while clearing P trace state

Currently, between the forEachP that ensures every P has a status in the
trace and readying dead Ps for the next generation, there's a big window
where GOMAXPROCS could run and change the number of Ps. In such
circumstances, P state will not get properly prepared for the next
generation, and we may fail to emit a status for a P.

This change addresses the problem by holding worldsema across both
operations. It also moves the status emission and state clearing to
before the generation transition because that makes the code *much*
clearer.

Currently we do both these things after the generation transition
targeting the next-next generation. The reason for this is to avoid an
extra forEachP (because we already handle P status in the STW for
enabling tracing to begin with) but approach is just plain harder to
reason about. Preparing the next generation before transitioning to it,
like we do for goroutines, is much clearer. Furthermore, we also need to
set up the dead P state even if we're stopping the trace, which would
mean duplicating code into both branches (if stopping the trace, then we
go non-preemptible, otherwise we do it under worldsema) which is also
less clear.

Note that with this change we no longer need to emit the P statuses
during the STW, which was probably the worse choice anyway, since
holding up the STW for an O(P) operation is worse than a forEachP we're
going to do anyway. So, this change does away with that too.

Fixes #76572.

Change-Id: Ie7908adff43a8a372cae432bcc2f4e0d6a87d7bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/729023
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>

diff --git a/src/runtime/trace.go b/src/runtime/trace.go
index 0fdc829f71e143603d97f776978991251855c7fb..a7e8937a05647a43adffbd12a53fbf3c49285451 100644 (file)
--- a/src/runtime/trace.go
+++ b/src/runtime/trace.go
@@ -440,11 +440,6 @@ func StartTrace() error {
 
        // Record the heap goal so we have it at the very beginning of the trace.
        tl.HeapGoal()
-
-       // Make sure a ProcStatus is emitted for every P, while we're here.
-       for _, pp := range allp {
-               tl.writer().writeProcStatusForP(pp, pp == tl.mp.p.ptr()).end()
-       }
        traceRelease(tl)
 
        unlock(&sched.sysmonlock)
@@ -553,14 +548,41 @@ func traceAdvance(stopTrace bool) {
        // here to minimize the time that we prevent the world from stopping.
        frequency := traceClockUnitsPerSecond()
 
-       // Now that we've done some of the heavy stuff, prevent the world from stopping.
+       // Prevent the world from stopping.
+       //
        // This is necessary to ensure the consistency of the STW events. If we're feeling
        // adventurous we could lift this restriction and add a STWActive event, but the
-       // cost of maintaining this consistency is low. We're not going to hold this semaphore
-       // for very long and most STW periods are very short.
-       // Once we hold worldsema, prevent preemption as well so we're not interrupted partway
-       // through this. We want to get this done as soon as possible.
+       // cost of maintaining this consistency is low.
+       //
+       // This is also a good time to preempt all the Ps and ensure they had a status traced.
        semacquire(&worldsema)
+
+       // Go over each P and emit a status event for it if necessary.
+       //
+       // TODO(mknyszek): forEachP is very heavyweight. We could do better by integrating
+       // the statusWasTraced check into it, to avoid preempting unnecessarily.
+       forEachP(waitReasonTraceProcStatus, func(pp *p) {
+               tl := traceAcquire()
+               if !pp.trace.statusWasTraced(tl.gen) {
+                       tl.writer().writeProcStatusForP(pp, false).end()
+               }
+               traceRelease(tl)
+       })
+
+       // While we're still holding worldsema (preventing a STW and thus a
+       // change in the number of Ps), reset the status on dead Ps.
+       // They just appear as idle.
+       //
+       // TODO(mknyszek): Consider explicitly emitting ProcCreate and ProcDestroy
+       // events to indicate whether a P exists, rather than just making its
+       // existence implicit.
+       for _, pp := range allp[len(allp):cap(allp)] {
+               pp.trace.readyNextGen(gen)
+       }
+
+       // Prevent preemption to make sure we're not interrupted.
+       //
+       // We want to get through the rest as soon as possible.
        mp := acquirem()
 
        // Advance the generation or stop the trace.
@@ -742,20 +764,6 @@ func traceAdvance(stopTrace bool) {
                unlock(&trace.lock)
        })
 
-       // Perform status reset on dead Ps because they just appear as idle.
-       //
-       // Preventing preemption is sufficient to access allp safely. allp is only
-       // mutated by GOMAXPROCS calls, which require a STW.
-       //
-       // TODO(mknyszek): Consider explicitly emitting ProcCreate and ProcDestroy
-       // events to indicate whether a P exists, rather than just making its
-       // existence implicit.
-       mp = acquirem()
-       for _, pp := range allp[len(allp):cap(allp)] {
-               pp.trace.readyNextGen(traceNextGen(gen))
-       }
-       releasem(mp)
-
        if stopTrace {
                // Acquire the shutdown sema to begin the shutdown process.
                semacquire(&traceShutdownSema)
@@ -772,23 +780,6 @@ func traceAdvance(stopTrace bool) {
                        trace.enabledWithAllocFree = false
                        debug.malloc = trace.debugMalloc
                }
-       } else {
-               // Go over each P and emit a status event for it if necessary.
-               //
-               // We do this at the beginning of the new generation instead of the
-               // end like we do for goroutines because forEachP doesn't give us a
-               // hook to skip Ps that have already been traced. Since we have to
-               // preempt all Ps anyway, might as well stay consistent with StartTrace
-               // which does this during the STW.
-               semacquire(&worldsema)
-               forEachP(waitReasonTraceProcStatus, func(pp *p) {
-                       tl := traceAcquire()
-                       if !pp.trace.statusWasTraced(tl.gen) {
-                               tl.writer().writeProcStatusForP(pp, false).end()
-                       }
-                       traceRelease(tl)
-               })
-               semrelease(&worldsema)
        }
 
        // Block until the trace reader has finished processing the last generation.