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 <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

5 files changed, 13 insertions, 62 deletions

diff --git a/src/runtime/debug.go b/src/runtime/debug.go
index 76eeb2e41a..af5c3a1170 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 0bc5568442..604d7d09b4 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 6da9689703..2a91e82185 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 9aa9facabe..67a84425a8 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 e3608c687f..62c06e67d9 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},