runtime: clean up dead P trace state when disabling tracing too

Right now, we're careful to clean up dead P state when we advance to
future trace generations. If we don't, then if that P comes back to
life, we might end up using its old stale trace state.

Unfortunately, we never handled this in the case when tracing stops,
only when advancing to new generations. As a result, stopping a trace,
starting it again, and then bringing a P back to life in the following
generation meant that the dead P could be using stale state.

Fixes #65318.

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

diff --git a/src/runtime/trace2.go b/src/runtime/trace2.go
index 2ac58405a367f5641e4b301d321ad5cd756654ef..6d6d4363a914a1057f6f2fbb6b0be3aabe7926d5 100644 (file)
--- a/src/runtime/trace2.go
+++ b/src/runtime/trace2.go
@@ -565,7 +565,22 @@ 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)
 
                // Finish off CPU profile reading.
@@ -586,16 +601,6 @@ func traceAdvance(stopTrace bool) {
                        }
                        traceRelease(tl)
                })
-               // Perform status reset on dead Ps because they just appear as idle.
-               //
-               // Holding worldsema prevents allp from changing.
-               //
-               // 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(traceNextGen(gen))
-               }
                semrelease(&worldsema)
        }