From 64c22b70bf00e15615bb17c29f808b55bc339682 Mon Sep 17 00:00:00 2001 From: Michael Knyszek Date: Fri, 24 Jan 2020 16:51:11 +0000 Subject: [PATCH] Revert "runtime: don't hold worldsema across mark phase" This reverts commit 7b294cdd8df0a9523010f6ffc80c59e64578f34b, CL 182657. Reason for revert: This change may be causing latency problems for applications which call ReadMemStats, because it may cause all goroutines to stop until the GC completes. https://golang.org/cl/215157 fixes this problem, but it's too late in the cycle to land that. Updates #19812. Change-Id: Iaa26f4dec9b06b9db2a771a44e45f58d0aa8f26d Reviewed-on: https://go-review.googlesource.com/c/go/+/216358 Run-TryBot: Michael Knyszek TryBot-Result: Gobot Gobot Reviewed-by: Cherry Zhang --- src/runtime/debug.go | 4 +-- src/runtime/mgc.go | 9 ------ src/runtime/proc.go | 44 +++------------------------ src/runtime/trace.go | 17 +++++------ src/runtime/trace/trace_stack_test.go | 1 - 5 files changed, 13 insertions(+), 62 deletions(-) diff --git a/src/runtime/debug.go b/src/runtime/debug.go index 76eeb2e41afd82..af5c3a11709fad 100644 --- a/src/runtime/debug.go +++ b/src/runtime/debug.go @@ -26,12 +26,12 @@ func GOMAXPROCS(n int) int { return ret } - stopTheWorldGC("GOMAXPROCS") + stopTheWorld("GOMAXPROCS") // newprocs will be processed by startTheWorld newprocs = int32(n) - startTheWorldGC() + startTheWorld() return ret } diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go index 0bc55684420d21..604d7d09b4831d 100644 --- a/src/runtime/mgc.go +++ b/src/runtime/mgc.go @@ -1269,7 +1269,6 @@ func gcStart(trigger gcTrigger) { } // Ok, we're doing it! Stop everybody else - semacquire(&gcsema) semacquire(&worldsema) if trace.enabled { @@ -1375,7 +1374,6 @@ func gcStart(trigger gcTrigger) { Gosched() } - semrelease(&worldsema) semrelease(&work.startSema) } @@ -1438,10 +1436,6 @@ top: return } - // forEachP needs worldsema to execute, and we'll need it to - // stop the world later, so acquire worldsema now. - semacquire(&worldsema) - // Flush all local buffers and collect flushedWork flags. gcMarkDoneFlushed = 0 systemstack(func() { @@ -1502,7 +1496,6 @@ top: // work to do. Keep going. It's possible the // transition condition became true again during the // ragged barrier, so re-check it. - semrelease(&worldsema) goto top } @@ -1579,7 +1572,6 @@ top: now := startTheWorldWithSema(true) work.pauseNS += now - work.pauseStart }) - semrelease(&worldsema) goto top } } @@ -1797,7 +1789,6 @@ func gcMarkTermination(nextTriggerRatio float64) { } semrelease(&worldsema) - semrelease(&gcsema) // Careful: another GC cycle may start now. releasem(mp) diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 6da9689703f692..2a91e821857bdd 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -857,23 +857,8 @@ func casGFromPreempted(gp *g, old, new uint32) bool { // goroutines. func stopTheWorld(reason string) { semacquire(&worldsema) - gp := getg() - gp.m.preemptoff = reason - systemstack(func() { - // Mark the goroutine which called stopTheWorld preemptible so its - // stack may be scanned. - // This lets a mark worker scan us while we try to stop the world - // since otherwise we could get in a mutual preemption deadlock. - // We must not modify anything on the G stack because a stack shrink - // may occur. A stack shrink is otherwise OK though because in order - // to return from this function (and to leave the system stack) we - // must have preempted all goroutines, including any attempting - // to scan our stack, in which case, any stack shrinking will - // have already completed by the time we exit. - casgstatus(gp, _Grunning, _Gwaiting) - stopTheWorldWithSema() - casgstatus(gp, _Gwaiting, _Grunning) - }) + getg().m.preemptoff = reason + systemstack(stopTheWorldWithSema) } // startTheWorld undoes the effects of stopTheWorld. @@ -885,31 +870,10 @@ func startTheWorld() { getg().m.preemptoff = "" } -// stopTheWorldGC has the same effect as stopTheWorld, but blocks -// until the GC is not running. It also blocks a GC from starting -// until startTheWorldGC is called. -func stopTheWorldGC(reason string) { - semacquire(&gcsema) - stopTheWorld(reason) -} - -// startTheWorldGC undoes the effects of stopTheWorldGC. -func startTheWorldGC() { - startTheWorld() - semrelease(&gcsema) -} - -// Holding worldsema grants an M the right to try to stop the world. +// Holding worldsema grants an M the right to try to stop the world +// and prevents gomaxprocs from changing concurrently. var worldsema uint32 = 1 -// Holding gcsema grants the M the right to block a GC, and blocks -// until the current GC is done. In particular, it prevents gomaxprocs -// from changing concurrently. -// -// TODO(mknyszek): Once gomaxprocs and the execution tracer can handle -// being changed/enabled during a GC, remove this. -var gcsema uint32 = 1 - // stopTheWorldWithSema is the core implementation of stopTheWorld. // The caller is responsible for acquiring worldsema and disabling // preemption first and then should stopTheWorldWithSema on the system diff --git a/src/runtime/trace.go b/src/runtime/trace.go index 9aa9facabee437..67a84425a8895c 100644 --- a/src/runtime/trace.go +++ b/src/runtime/trace.go @@ -180,12 +180,9 @@ func traceBufPtrOf(b *traceBuf) traceBufPtr { // Most clients should use the runtime/trace package or the testing package's // -test.trace flag instead of calling StartTrace directly. func StartTrace() error { - // Stop the world so that we can take a consistent snapshot + // Stop the world, so that we can take a consistent snapshot // of all goroutines at the beginning of the trace. - // Do not stop the world during GC so we ensure we always see - // a consistent view of GC-related events (e.g. a start is always - // paired with an end). - stopTheWorldGC("start tracing") + stopTheWorld("start tracing") // We are in stop-the-world, but syscalls can finish and write to trace concurrently. // Exitsyscall could check trace.enabled long before and then suddenly wake up @@ -196,7 +193,7 @@ func StartTrace() error { if trace.enabled || trace.shutdown { unlock(&trace.bufLock) - startTheWorldGC() + startTheWorld() return errorString("tracing is already enabled") } @@ -267,7 +264,7 @@ func StartTrace() error { unlock(&trace.bufLock) - startTheWorldGC() + startTheWorld() return nil } @@ -276,14 +273,14 @@ func StartTrace() error { func StopTrace() { // Stop the world so that we can collect the trace buffers from all p's below, // and also to avoid races with traceEvent. - stopTheWorldGC("stop tracing") + stopTheWorld("stop tracing") // See the comment in StartTrace. lock(&trace.bufLock) if !trace.enabled { unlock(&trace.bufLock) - startTheWorldGC() + startTheWorld() return } @@ -320,7 +317,7 @@ func StopTrace() { trace.shutdown = true unlock(&trace.bufLock) - startTheWorldGC() + startTheWorld() // The world is started but we've set trace.shutdown, so new tracing can't start. // Wait for the trace reader to flush pending buffers and stop. diff --git a/src/runtime/trace/trace_stack_test.go b/src/runtime/trace/trace_stack_test.go index e3608c687fa703..62c06e67d9d374 100644 --- a/src/runtime/trace/trace_stack_test.go +++ b/src/runtime/trace/trace_stack_test.go @@ -233,7 +233,6 @@ func TestTraceSymbolize(t *testing.T) { }}, {trace.EvGomaxprocs, []frame{ {"runtime.startTheWorld", 0}, // this is when the current gomaxprocs is logged. - {"runtime.startTheWorldGC", 0}, {"runtime.GOMAXPROCS", 0}, {"runtime/trace_test.TestTraceSymbolize", 0}, {"testing.tRunner", 0},