runtime: simplify gcBgMarkWorker preemption

gcBgMarkWorker G's are primarily scheduled by findRunnableGCWorker, but
that no longer needs to be strictly enforced. Temporary preemption to a
runq is fine when the P is not in use.

We still releasem in gopark in the normal case for efficiency: if
gcDrain stops because gp.preempt is set, then gopark would always
preempt. That is fine, but inefficient, since it will reschedule simply
to park again. Thus, we keep releasem in unlockf to skip this extra
cycle.

Change-Id: I6d1a42e3ca41b76227142a6b5bfb376c9213e3c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/262349
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Trust: Michael Pratt <mpratt@google.com>

1 files changed, 45 insertions, 22 deletions

diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go
index fabb846a74..9d2682f03c 100644
--- a/src/runtime/mgc.go
+++ b/src/runtime/mgc.go
@@ -1829,8 +1829,12 @@ func gcBgMarkStartWorkers() {
 	// again, we can reuse the old workers; no need to create new workers.
 	for gcBgMarkWorkerCount < gomaxprocs {
 		go gcBgMarkWorker()
+
 		notetsleepg(&work.bgMarkReady, -1)
 		noteclear(&work.bgMarkReady)
+		// The worker is now guaranteed to be added to the pool before
+		// its P's next findRunnableGCWorker.
+
 		gcBgMarkWorkerCount++
 	}
 }
@@ -1877,32 +1881,55 @@ func gcBgMarkWorker() {
 	gp.m.preemptoff = ""
 
 	node.gp.set(gp)
+
 	node.m.set(acquirem())
-	// Inform gcBgMarkStartWorkers that this worker is ready. After this
-	// point, the background mark worker is scheduled cooperatively by
-	// gcController.findRunnableGCWorker. Hence, it must never be
-	// preempted, as this would put it into _Grunnable and put it on a run
-	// queue. Instead, when the preempt flag is set, this puts itself into
-	// _Gwaiting to be woken up by gcController.findRunnableGCWorker at the
-	// appropriate time.
 	notewakeup(&work.bgMarkReady)
+	// After this point, the background mark worker is generally scheduled
+	// cooperatively by gcController.findRunnableGCWorker. While performing
+	// work on the P, preemption is disabled because we are working on
+	// P-local work buffers. When the preempt flag is set, this puts itself
+	// into _Gwaiting to be woken up by gcController.findRunnableGCWorker
+	// at the appropriate time.
+	//
+	// When preemption is enabled (e.g., while in gcMarkDone), this worker
+	// may be preempted and schedule as a _Grunnable G from a runq. That is
+	// fine; it will eventually gopark again for further scheduling via
+	// findRunnableGCWorker.
+	//
+	// Since we disable preemption before notifying bgMarkReady, we
+	// guarantee that this G will be in the worker pool for the next
+	// findRunnableGCWorker. This isn't strictly necessary, but it reduces
+	// latency between _GCmark starting and the workers starting.
 
 	for {
 		// Go to sleep until woken by
-		// gcController.findRunnableGCWorker.  We can't releasem yet
-		// since even the call to gopark may be preempted.
+		// gcController.findRunnableGCWorker.
 		gopark(func(g *g, nodep unsafe.Pointer) bool {
 			node := (*gcBgMarkWorkerNode)(nodep)
 
-			// The worker G is no longer running, so it's
-			// now safe to allow preemption.
-			releasem(node.m.ptr())
+			if mp := node.m.ptr(); mp != nil {
+				// The worker G is no longer running; release
+				// the M.
+				//
+				// N.B. it is _safe_ to release the M as soon
+				// as we are no longer performing P-local mark
+				// work.
+				//
+				// However, since we cooperatively stop work
+				// when gp.preempt is set, if we releasem in
+				// the loop then the following call to gopark
+				// would immediately preempt the G. This is
+				// also safe, but inefficient: the G must
+				// schedule again only to enter gopark and park
+				// again. Thus, we defer the release until
+				// after parking the G.
+				releasem(mp)
+			}
 
 			// Release this G to the pool.
 			gcBgMarkWorkerPool.push(&node.node)
 			// Note that at this point, the G may immediately be
 			// rescheduled and may be running.
-
 			return true
 		}, unsafe.Pointer(node), waitReasonGCWorkerIdle, traceEvGoBlock, 0)
 
@@ -1913,7 +1940,7 @@ func gcBgMarkWorker() {
 		// scheduler wants to preempt us, we'll stop draining,
 		// dispose the gcw, and then preempt.
 		node.m.set(acquirem())
-		pp := gp.m.p.ptr()  // P can't change with preemption disabled.
+		pp := gp.m.p.ptr() // P can't change with preemption disabled.
 
 		if gcBlackenEnabled == 0 {
 			println("worker mode", pp.gcMarkWorkerMode)
@@ -2005,17 +2032,13 @@ func gcBgMarkWorker() {
 		// If this worker reached a background mark completion
 		// point, signal the main GC goroutine.
 		if incnwait == work.nproc && !gcMarkWorkAvailable(nil) {
-			// Make this G preemptible since we are done with per-P
-			// work.
+			// We don't need the P-local buffers here, allow
+			// preemption becuse we may schedule like a regular
+			// goroutine in gcMarkDone (block on locks, etc).
 			releasem(node.m.ptr())
+			node.m.set(nil)
 
 			gcMarkDone()
-
-			// Disable preemption and prepare to park.
-			//
-			// Note that we may be running on a different P at this
-			// point, so we can't use pp.
-			node.m.set(acquirem())
 		}
 	}
 }