From 52b5f164ae65b96ce9bd0fb7d06c609bf62c3d30 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Thu, 8 Feb 2024 17:16:49 +0000 Subject: [PATCH] runtime: make checking if tracing is enabled non-atomic Tracing is currently broken when using iter.Pull from the rangefunc experiment partly because the "tracing is off" fast path in traceAcquire was deemed too expensive to check (an atomic load) during the coroutine switch. This change adds trace.enabled, a non-atomic indicator of whether tracing is enabled. It doubles trace.gen, which is the source of truth on whether tracing is enabled. The semantics around trace.enabled are subtle. When tracing is enabled, we need to be careful to make sure that if gen != 0, goroutines enter the tracer on traceAcquire. This is enforced by making sure trace.enabled is published atomically with trace.gen. The STW takes care of synchronization with most Ms, but there's still sysmon and goroutines exiting syscalls. We need to synchronize with those explicitly anyway, which luckily takes care of trace.enabled as well. When tracing is disabled, it's always OK for trace.enabled to be stale, since traceAcquire will always double-check gen before proceeding. For #61897. Change-Id: I47c2a530fb5339c15e419312fbb1e22d782cd453 Reviewed-on: https://go-review.googlesource.com/c/go/+/565935 Auto-Submit: Michael Knyszek LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Pratt --- src/runtime/trace2.go | 36 +++++++++++++++++++++++++++++++++--- src/runtime/trace2runtime.go | 2 +- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/runtime/trace2.go b/src/runtime/trace2.go index 673205dda8a4a..2ac58405a367f 100644 --- a/src/runtime/trace2.go +++ b/src/runtime/trace2.go @@ -98,6 +98,20 @@ var trace struct { goStopReasons [2][len(traceGoStopReasonStrings)]traceArg goBlockReasons [2][len(traceBlockReasonStrings)]traceArg + // enabled indicates whether tracing is enabled, but it is only an optimization, + // NOT the source of truth on whether tracing is enabled. Tracing is only truly + // enabled if gen != 0. This is used as an optimistic fast path check. + // + // Transitioning this value from true -> false is easy (once gen is 0) + // because it's OK for enabled to have a stale "true" value. traceAcquire will + // always double-check gen. + // + // Transitioning this value from false -> true is harder. We need to make sure + // this is observable as true strictly before gen != 0. To maintain this invariant + // we only make this transition with the world stopped and use the store to gen + // as a publication barrier. + enabled bool + // Trace generation counter. gen atomic.Uintptr lastNonZeroGen uintptr // last non-zero value of gen @@ -211,8 +225,19 @@ func StartTrace() error { // Start tracing. // - // After this executes, other Ms may start creating trace buffers and emitting + // Set trace.enabled. This is *very* subtle. We need to maintain the invariant that if + // trace.gen != 0, then trace.enabled is always observed as true. Simultaneously, for + // performance, we need trace.enabled to be read without any synchronization. + // + // We ensure this is safe by stopping the world, which acts a global barrier on almost + // every M, and explicitly synchronize with any other Ms that could be running concurrently + // with us. Today, there are only two such cases: + // - sysmon, which we synchronized with by acquiring sysmonlock. + // - goroutines exiting syscalls, which we synchronize with via trace.exitingSyscall. + // + // After trace.gen is updated, other Ms may start creating trace buffers and emitting // data into them. + trace.enabled = true trace.gen.Store(firstGen) // Wait for exitingSyscall to drain. @@ -222,8 +247,9 @@ func StartTrace() error { // goroutines to run on. // // Because we set gen before checking this, and because exitingSyscall is always incremented - // *after* traceAcquire (which checks gen), we can be certain that when exitingSyscall is zero - // that any goroutine that goes to exit a syscall from then on *must* observe the new gen. + // *before* traceAcquire (which checks gen), we can be certain that when exitingSyscall is zero + // that any goroutine that goes to exit a syscall from then on *must* observe the new gen as + // well as trace.enabled being set to true. // // The critical section on each goroutine here is going to be quite short, so the likelihood // that we observe a zero value is high. @@ -376,6 +402,10 @@ func traceAdvance(stopTrace bool) { trace.shutdown.Store(true) trace.gen.Store(0) unlock(&trace.lock) + + // Clear trace.enabled. It is totally OK for this value to be stale, + // because traceAcquire will always double-check gen. + trace.enabled = false }) } else { trace.gen.Store(traceNextGen(gen)) diff --git a/src/runtime/trace2runtime.go b/src/runtime/trace2runtime.go index 512e53907e60b..7b88c258bacdd 100644 --- a/src/runtime/trace2runtime.go +++ b/src/runtime/trace2runtime.go @@ -141,7 +141,7 @@ var traceGoStopReasonStrings = [...]string{ // //go:nosplit func traceEnabled() bool { - return trace.gen.Load() != 0 + return trace.enabled } // traceShuttingDown returns true if the trace is currently shutting down.