runtime: fix sweep termination condition

Currently, there is a chance that the sweep termination condition could
flap, causing e.g. runtime.GC to return before all sweep work has not
only been drained, but also completed. CL 307915 and CL 307916 attempted
to fix this problem, but it is still possible that mheap_.sweepDrained is
marked before any outstanding sweepers are accounted for in
mheap_.sweepers, leaving a window in which a thread could observe
isSweepDone as true before it actually was (and after some time it would
revert to false, then true again, depending on the number of outstanding
sweepers at that point).

This change fixes the sweep termination condition by merging
mheap_.sweepers and mheap_.sweepDrained into a single atomic value.

This value is updated such that a new potential sweeper will increment
the oustanding sweeper count iff there are still outstanding spans to be
swept without an outstanding sweeper to pick them up. This design
simplifies the sweep termination condition into a single atomic load and
comparison and ensures the condition never flaps.

Updates #46500.
Fixes #45315.

Change-Id: I6d69aff156b8d48428c4cc8cfdbf28be346dbf04
Reviewed-on: https://go-review.googlesource.com/c/go/+/333389
Trust: Michael Knyszek <[email protected]>
Run-TryBot: Michael Knyszek <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Austin Clements <[email protected]>

Author: mknyszek

src/runtime/mcentral.go

@@ -102,59 +102,62 @@ func (c *mcentral) cacheSpan() *mspan {
 	spanBudget := 100
 
 	var s *mspan
-	sl := newSweepLocker()
-	sg := sl.sweepGen
+	var sl sweepLocker
 
 	// Try partial swept spans first.
+	sg := mheap_.sweepgen
 	if s = c.partialSwept(sg).pop(); s != nil {
 		goto havespan
 	}
 
-	// Now try partial unswept spans.
-	for ; spanBudget >= 0; spanBudget-- {
-		s = c.partialUnswept(sg).pop()
-		if s == nil {
-			break
-		}
-		if s, ok := sl.tryAcquire(s); ok {
-			// We got ownership of the span, so let's sweep it and use it.
-			s.sweep(true)
-			sl.dispose()
-			goto havespan
-		}
-		// We failed to get ownership of the span, which means it's being or
-		// has been swept by an asynchronous sweeper that just couldn't remove it
-		// from the unswept list. That sweeper took ownership of the span and
-		// responsibility for either freeing it to the heap or putting it on the
-		// right swept list. Either way, we should just ignore it (and it's unsafe
-		// for us to do anything else).
-	}
-	// Now try full unswept spans, sweeping them and putting them into the
-	// right list if we fail to get a span.
-	for ; spanBudget >= 0; spanBudget-- {
-		s = c.fullUnswept(sg).pop()
-		if s == nil {
-			break
-		}
-		if s, ok := sl.tryAcquire(s); ok {
-			// We got ownership of the span, so let's sweep it.
-			s.sweep(true)
-			// Check if there's any free space.
-			freeIndex := s.nextFreeIndex()
-			if freeIndex != s.nelems {
-				s.freeindex = freeIndex
-				sl.dispose()
+	sl = sweep.active.begin()
+	if sl.valid {
+		// Now try partial unswept spans.
+		for ; spanBudget >= 0; spanBudget-- {
+			s = c.partialUnswept(sg).pop()
+			if s == nil {
+				break
+			}
+			if s, ok := sl.tryAcquire(s); ok {
+				// We got ownership of the span, so let's sweep it and use it.
+				s.sweep(true)
+				sweep.active.end(sl)
 				goto havespan
 			}
-			// Add it to the swept list, because sweeping didn't give us any free space.
-			c.fullSwept(sg).push(s.mspan)
+			// We failed to get ownership of the span, which means it's being or
+			// has been swept by an asynchronous sweeper that just couldn't remove it
+			// from the unswept list. That sweeper took ownership of the span and
+			// responsibility for either freeing it to the heap or putting it on the
+			// right swept list. Either way, we should just ignore it (and it's unsafe
+			// for us to do anything else).
+		}
+		// Now try full unswept spans, sweeping them and putting them into the
+		// right list if we fail to get a span.
+		for ; spanBudget >= 0; spanBudget-- {
+			s = c.fullUnswept(sg).pop()
+			if s == nil {
+				break
+			}
+			if s, ok := sl.tryAcquire(s); ok {
+				// We got ownership of the span, so let's sweep it.
+				s.sweep(true)
+				// Check if there's any free space.
+				freeIndex := s.nextFreeIndex()
+				if freeIndex != s.nelems {
+					s.freeindex = freeIndex
+					sweep.active.end(sl)
+					goto havespan
+				}
+				// Add it to the swept list, because sweeping didn't give us any free space.
+				c.fullSwept(sg).push(s.mspan)
+			}
+			// See comment for partial unswept spans.
+		}
+		sweep.active.end(sl)
+		if trace.enabled {
+			traceGCSweepDone()
+			traceDone = true
 		}
-		// See comment for partial unswept spans.
-	}
-	sl.dispose()
-	if trace.enabled {
-		traceGCSweepDone()
-		traceDone = true
 	}
 
 	// We failed to get a span from the mcentral so get one from mheap.

src/runtime/mgc.go

@@ -154,7 +154,7 @@ func gcinit() {
 		throw("size of Workbuf is suboptimal")
 	}
 	// No sweep on the first cycle.
-	mheap_.sweepDrained = 1
+	sweep.active.state.Store(sweepDrainedMask)
 
 	// Initialize GC pacer state.
 	// Use the environment variable GOGC for the initial gcPercent value.
@@ -1022,8 +1022,10 @@ func gcMarkTermination(nextTriggerRatio float64) {
 	// 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.
-	sl := newSweepLocker()
-	sl.blockCompletion()
+	sl := sweep.active.begin()
+	if !sl.valid {
+		throw("failed to set sweep barrier")
+	}
 
 	systemstack(func() { startTheWorldWithSema(true) })
 
@@ -1050,7 +1052,7 @@ func gcMarkTermination(nextTriggerRatio float64) {
 	})
 	// Now that we've swept stale spans in mcaches, they don't
 	// count against unswept spans.
-	sl.dispose()
+	sweep.active.end(sl)
 
 	// Print gctrace before dropping worldsema. As soon as we drop
 	// worldsema another cycle could start and smash the stats
@@ -1457,7 +1459,7 @@ func gcSweep(mode gcMode) {
 
 	lock(&mheap_.lock)
 	mheap_.sweepgen += 2
-	mheap_.sweepDrained = 0
+	sweep.active.reset()
 	mheap_.pagesSwept.Store(0)
 	mheap_.sweepArenas = mheap_.allArenas
 	mheap_.reclaimIndex.Store(0)