]> Cypherpunks.ru repositories - gostls13.git/commitdiff
Revert "runtime: don't hold worldsema across mark phase"
authorMichael Knyszek <mknyszek@google.com>
Fri, 24 Jan 2020 16:51:11 +0000 (16:51 +0000)
committerMichael Knyszek <mknyszek@google.com>
Fri, 24 Jan 2020 23:27:33 +0000 (23:27 +0000)
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 <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
src/runtime/debug.go
src/runtime/mgc.go
src/runtime/proc.go
src/runtime/trace.go
src/runtime/trace/trace_stack_test.go

index 76eeb2e41afd82049dfbcd46cd36c333286c268b..af5c3a11709fad64ffa2d31b14ae3185096016c6 100644 (file)
@@ -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
 }
 
index 0bc55684420d213a3e5c233245f2ac292977a5aa..604d7d09b4831d2413a8a2053f49540e9e75b2de 100644 (file)
@@ -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)
index 6da9689703f6927ebf9075a86cf157d8e146bfd3..2a91e821857bdda16c89bc85a5abae78a82936e7 100644 (file)
@@ -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
index 9aa9facabee43728565581fa146240bdbb11ebc5..67a84425a8895c9916928924e0eda3f4fe4c926c 100644 (file)
@@ -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.
index e3608c687fa70311d407a9452e3d7b3fdf9eda9b..62c06e67d9d37436ec34c5469f5eb01f6eff63fa 100644 (file)
@@ -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},