runtime: fix debug non-concurrent sweep mode after activeSweep changes

Currently the GC creates a sweepLocker before restarting the world at
the end of the mark phase, so that it can safely flush mcaches without
the runtime incorrectly concluding that sweeping is done before that
happens.

However, with GODEBUG=gcstoptheworld=2, where sweeping happens during
that STW phase, creating that sweepLocker will fail, since the runtime
will conclude that sweeping is in fact complete (all the queues will be
drained). The problem however is that gcSweep, which does the
non-concurrent sweeping, doesn't actually flush mcaches.

In essence, this failure to create a sweepLocker is indicating a real
issue: sweeping is marked as complete, but we haven't flush the mcaches
yet!

The fix to this is to flush mcaches in gcSweep when in a non-concurrent
sweep. Now that gcSweep actually completes a full sweep, it's safe to
ignore a failure to create a sweepLocker (and in fact, it *must* fail).

While we're here, let's also remove _ConcurrentSweep, the debug flag.
There's already an alias for it called concurrentSweep, and there's only
one use of it in gcSweep.

Lastly, add a dist test for the GODEBUG=gcstoptheworld=2 mode.

Fixes #53885.

Change-Id: I8a1e5b8f362ed8abd03f76e4950d3211f145ab1f
Reviewed-on: https://go-review.googlesource.com/c/go/+/479517
Run-TryBot: Michael Knyszek <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: mknyszek

src/cmd/dist/test.go

@@ -676,8 +676,22 @@ func (t *tester) registerTests() {
 			})
 	}
 
-	// morestack tests. We only run these on in long-test mode
-	// (with GO_TEST_SHORT=false) because the runtime test is
+	// GODEBUG=gcstoptheworld=2 tests. We only run these in long-test
+	// mode (with GO_TEST_SHORT=0) because this is just testing a
+	// non-critical debug setting.
+	if !t.compileOnly && !t.short {
+		t.registerTest("GODEBUG=gcstoptheworld=2 archive/zip",
+			&goTest{
+				variant: "runtime:gcstoptheworld2",
+				timeout: 300 * time.Second,
+				short:   true,
+				env:     []string{"GODEBUG=gcstoptheworld=2"},
+				pkg:     "archive/zip",
+			})
+	}
+
+	// morestack tests. We only run these in long-test mode
+	// (with GO_TEST_SHORT=0) because the runtime test is
 	// already quite long and mayMoreStackMove makes it about
 	// twice as slow.
 	if !t.compileOnly && !t.short {

src/runtime/malloc.go

@@ -117,8 +117,6 @@ const (
 	pageShift = _PageShift
 	pageSize  = _PageSize
 
-	concurrentSweep = _ConcurrentSweep
-
 	_PageSize = 1 << _PageShift
 	_PageMask = _PageSize - 1
 

src/runtime/mgc.go

@@ -135,9 +135,12 @@ import (
 )
 
 const (
-	_DebugGC         = 0
-	_ConcurrentSweep = true
-	_FinBlockSize    = 4 * 1024
+	_DebugGC      = 0
+	_FinBlockSize = 4 * 1024
+
+	// concurrentSweep is a debug flag. Disabling this flag
+	// ensures all spans are swept while the world is stopped.
+	concurrentSweep = true
 
 	// debugScanConservative enables debug logging for stack
 	// frames that are scanned conservatively.
@@ -969,6 +972,7 @@ func gcMarkTermination() {
 		// before continuing.
 	})
 
+	var stwSwept bool
 	systemstack(func() {
 		work.heap2 = work.bytesMarked
 		if debug.gccheckmark > 0 {
@@ -987,7 +991,7 @@ func gcMarkTermination() {
 
 		// marking is complete so we can turn the write barrier off
 		setGCPhase(_GCoff)
-		gcSweep(work.mode)
+		stwSwept = gcSweep(work.mode)
 	})
 
 	mp.traceback = 0
@@ -1079,9 +1083,19 @@ func gcMarkTermination() {
 	// Those aren't tracked in any sweep lists, so we need to
 	// count them against sweep completion until we ensure all
 	// those spans have been forced out.
+	//
+	// If gcSweep fully swept the heap (for example if the sweep
+	// is not concurrent due to a GODEBUG setting), then we expect
+	// the sweepLocker to be invalid, since sweeping is done.
+	//
+	// N.B. Below we might duplicate some work from gcSweep; this is
+	// fine as all that work is idempotent within a GC cycle, and
+	// we're still holding worldsema so a new cycle can't start.
 	sl := sweep.active.begin()
-	if !sl.valid {
+	if !stwSwept && !sl.valid {
 		throw("failed to set sweep barrier")
+	} else if stwSwept && sl.valid {
+		throw("non-concurrent sweep failed to drain all sweep queues")
 	}
 
 	systemstack(func() { startTheWorldWithSema() })
@@ -1123,9 +1137,15 @@ func gcMarkTermination() {
 			pp.pinnerCache = nil
 		})
 	})
-	// Now that we've swept stale spans in mcaches, they don't
-	// count against unswept spans.
-	sweep.active.end(sl)
+	if sl.valid {
+		// Now that we've swept stale spans in mcaches, they don't
+		// count against unswept spans.
+		//
+		// Note: this sweepLocker may not be valid if sweeping had
+		// already completed during the STW. See the corresponding
+		// begin() call that produced sl.
+		sweep.active.end(sl)
+	}
 
 	// Print gctrace before dropping worldsema. As soon as we drop
 	// worldsema another cycle could start and smash the stats
@@ -1538,10 +1558,12 @@ func gcMark(startTime int64) {
 // gcSweep must be called on the system stack because it acquires the heap
 // lock. See mheap for details.
 //
+// Returns true if the heap was fully swept by this function.
+//
 // The world must be stopped.
 //
 //go:systemstack
-func gcSweep(mode gcMode) {
+func gcSweep(mode gcMode) bool {
 	assertWorldStopped()
 
 	if gcphase != _GCoff {
@@ -1559,12 +1581,16 @@ func gcSweep(mode gcMode) {
 
 	sweep.centralIndex.clear()
 
-	if !_ConcurrentSweep || mode == gcForceBlockMode {
+	if !concurrentSweep || mode == gcForceBlockMode {
 		// Special case synchronous sweep.
 		// Record that no proportional sweeping has to happen.
 		lock(&mheap_.lock)
 		mheap_.sweepPagesPerByte = 0
 		unlock(&mheap_.lock)
+		// Flush all mcaches.
+		for _, pp := range allp {
+			pp.mcache.prepareForSweep()
+		}
 		// Sweep all spans eagerly.
 		for sweepone() != ^uintptr(0) {
 			sweep.npausesweep++
@@ -1578,7 +1604,7 @@ func gcSweep(mode gcMode) {
 		// available immediately.
 		mProf_NextCycle()
 		mProf_Flush()
-		return
+		return true
 	}
 
 	// Background sweep.
@@ -1588,6 +1614,7 @@ func gcSweep(mode gcMode) {
 		ready(sweep.g, 0, true)
 	}
 	unlock(&sweep.lock)
+	return false
 }
 
 // gcResetMarkState resets global state prior to marking (concurrent