From 9f13665088012298146c573bc2a7255b1caf2750 Mon Sep 17 00:00:00 2001 From: Michael Knyszek Date: Mon, 8 Apr 2024 21:34:24 +0000 Subject: [PATCH] Revert "runtime: push down systemstack requirement for tracer where possible" This reverts CL 572095. Reason for revert: Broke longtest builders. Change-Id: Iac3a8159d3afb4156a49c7d6819cdd15fe9d4bbb Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/577376 LUCI-TryBot-Result: Go LUCI Auto-Submit: Michael Knyszek Reviewed-by: Carlos Amedee --- src/runtime/trace2.go | 20 ++++++++++---------- src/runtime/trace2cpu.go | 15 +++++++++------ src/runtime/trace2stack.go | 13 ++++++++----- src/runtime/trace2string.go | 25 +++++++++++++------------ 4 files changed, 40 insertions(+), 33 deletions(-) diff --git a/src/runtime/trace2.go b/src/runtime/trace2.go index a9be4e1962..d516001433 100644 --- a/src/runtime/trace2.go +++ b/src/runtime/trace2.go @@ -551,17 +551,17 @@ func traceAdvance(stopTrace bool) { // Read everything out of the last gen's CPU profile buffer. traceReadCPU(gen) - // Flush CPU samples, stacks, and strings for the last generation. This is safe, - // because we're now certain no M is writing to the last generation. - // - // Ordering is important here. traceCPUFlush may generate new stacks and dumping - // stacks may generate new strings. - traceCPUFlush(gen) - trace.stackTab[gen%2].dump(gen) - trace.stringTab[gen%2].reset(gen) - - // That's it. This generation is done producing buffers. systemstack(func() { + // Flush CPU samples, stacks, and strings for the last generation. This is safe, + // because we're now certain no M is writing to the last generation. + // + // Ordering is important here. traceCPUFlush may generate new stacks and dumping + // stacks may generate new strings. + traceCPUFlush(gen) + trace.stackTab[gen%2].dump(gen) + trace.stringTab[gen%2].reset(gen) + + // That's it. This generation is done producing buffers. lock(&trace.lock) trace.flushedGen.Store(gen) unlock(&trace.lock) diff --git a/src/runtime/trace2cpu.go b/src/runtime/trace2cpu.go index 2bb6f903f5..b3b0fb046d 100644 --- a/src/runtime/trace2cpu.go +++ b/src/runtime/trace2cpu.go @@ -195,15 +195,18 @@ func traceReadCPU(gen uintptr) bool { // traceCPUFlush flushes trace.cpuBuf[gen%2]. The caller must be certain that gen // has completed and that there are no more writers to it. +// +// Must run on the systemstack because it flushes buffers and acquires trace.lock +// to do so. +// +//go:systemstack func traceCPUFlush(gen uintptr) { // Flush any remaining trace buffers containing CPU samples. if buf := trace.cpuBuf[gen%2]; buf != nil { - systemstack(func() { - lock(&trace.lock) - traceBufFlush(buf, gen) - unlock(&trace.lock) - trace.cpuBuf[gen%2] = nil - }) + lock(&trace.lock) + traceBufFlush(buf, gen) + unlock(&trace.lock) + trace.cpuBuf[gen%2] = nil } } diff --git a/src/runtime/trace2stack.go b/src/runtime/trace2stack.go index 7d698c89d3..4ee3b32b05 100644 --- a/src/runtime/trace2stack.go +++ b/src/runtime/trace2stack.go @@ -138,6 +138,11 @@ func (t *traceStackTable) put(pcs []uintptr) uint64 { // dump writes all previously cached stacks to trace buffers, // releases all memory and resets state. It must only be called once the caller // can guarantee that there are no more writers to the table. +// +// This must run on the system stack because it flushes buffers and thus +// may acquire trace.lock. +// +//go:systemstack func (t *traceStackTable) dump(gen uintptr) { w := unsafeTraceWriter(gen, nil) @@ -189,11 +194,9 @@ func (t *traceStackTable) dump(gen uintptr) { } // Still, hold the lock over reset. The callee expects it, even though it's // not strictly necessary. - systemstack(func() { - lock(&t.tab.lock) - t.tab.reset() - unlock(&t.tab.lock) - }) + lock(&t.tab.lock) + t.tab.reset() + unlock(&t.tab.lock) w.flush().end() } diff --git a/src/runtime/trace2string.go b/src/runtime/trace2string.go index 21ef5eaf98..cbb0ecfb37 100644 --- a/src/runtime/trace2string.go +++ b/src/runtime/trace2string.go @@ -49,7 +49,7 @@ func (t *traceStringTable) emit(gen uintptr, s string) uint64 { // writeString writes the string to t.buf. // -// Must run on the systemstack because it acquires t.lock. +// Must run on the systemstack because it may flush buffers and thus could acquire trace.lock. // //go:systemstack func (t *traceStringTable) writeString(gen uintptr, id uint64, s string) { @@ -75,7 +75,7 @@ func (t *traceStringTable) writeString(gen uintptr, id uint64, s string) { w.varint(uint64(len(s))) w.stringData(s) - // Store back buf in case it was updated during ensure. + // Store back buf if it was updated during ensure. t.buf = w.traceBuf unlock(&t.lock) } @@ -84,20 +84,21 @@ func (t *traceStringTable) writeString(gen uintptr, id uint64, s string) { // // Must be called only once the caller is certain nothing else will be // added to this table. +// +// Because it flushes buffers, this may acquire trace.lock and thus +// must run on the systemstack. +// +//go:systemstack func (t *traceStringTable) reset(gen uintptr) { if t.buf != nil { - systemstack(func() { - lock(&trace.lock) - traceBufFlush(t.buf, gen) - unlock(&trace.lock) - }) + lock(&trace.lock) + traceBufFlush(t.buf, gen) + unlock(&trace.lock) t.buf = nil } // Reset the table. - systemstack(func() { - lock(&t.tab.lock) - t.tab.reset() - unlock(&t.tab.lock) - }) + lock(&t.tab.lock) + t.tab.reset() + unlock(&t.tab.lock) } -- 2.48.1