Skip to content

Commit ccfcc30

Browse files
mknyszekgopherbot
authored andcommitted
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 <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Michael Knyszek <[email protected]>
1 parent 7348773 commit ccfcc30

File tree

1 file changed

+15
-10
lines changed

1 file changed

+15
-10
lines changed

src/runtime/trace2.go

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,22 @@ func traceAdvance(stopTrace bool) {
565565
unlock(&trace.lock)
566566
})
567567

568+
// Perform status reset on dead Ps because they just appear as idle.
569+
//
570+
// Preventing preemption is sufficient to access allp safely. allp is only
571+
// mutated by GOMAXPROCS calls, which require a STW.
572+
//
573+
// TODO(mknyszek): Consider explicitly emitting ProcCreate and ProcDestroy
574+
// events to indicate whether a P exists, rather than just making its
575+
// existence implicit.
576+
mp = acquirem()
577+
for _, pp := range allp[len(allp):cap(allp)] {
578+
pp.trace.readyNextGen(traceNextGen(gen))
579+
}
580+
releasem(mp)
581+
568582
if stopTrace {
583+
// Acquire the shutdown sema to begin the shutdown process.
569584
semacquire(&traceShutdownSema)
570585

571586
// Finish off CPU profile reading.
@@ -586,16 +601,6 @@ func traceAdvance(stopTrace bool) {
586601
}
587602
traceRelease(tl)
588603
})
589-
// Perform status reset on dead Ps because they just appear as idle.
590-
//
591-
// Holding worldsema prevents allp from changing.
592-
//
593-
// TODO(mknyszek): Consider explicitly emitting ProcCreate and ProcDestroy
594-
// events to indicate whether a P exists, rather than just making its
595-
// existence implicit.
596-
for _, pp := range allp[len(allp):cap(allp)] {
597-
pp.trace.readyNextGen(traceNextGen(gen))
598-
}
599604
semrelease(&worldsema)
600605
}
601606

0 commit comments

Comments
 (0)