From ccfcc30f3e3ce90bad0b7a40f79eee78da6adb47 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Mon, 26 Feb 2024 20:36:29 +0000 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI Auto-Submit: Michael Knyszek --- src/runtime/trace2.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/runtime/trace2.go b/src/runtime/trace2.go index 2ac58405a3..6d6d4363a9 100644 --- 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) } -- 2.48.1