[release-branch.go1.19] runtime: check for overflow in sweep assist

The sweep assist computation is intentionally racy for performance,
since the specifics of sweep assist aren't super sensitive to error.
However, if overflow occurs when computing the live heap delta, we can
end up with a massive sweep target that causes the sweep assist to sweep
until sweep termination, causing severe latency issues. In fact, because
heapLive doesn't always increase monotonically then anything that
flushes mcaches will cause _all_ allocating goroutines to inevitably get
stuck in sweeping.

Consider the following scenario:
1. SetGCPercent is called, updating sweepHeapLiveBasis to heapLive.
2. Very shortly after, ReadMemStats is called, flushing mcaches and
   decreasing heapLive below the value sweepHeapLiveBasis was set to.
3. Every allocating goroutine goes to refill its mcache, calls into
   deductSweepCredit for sweep assist, and gets stuck sweeping until
   the sweep phase ends.

Fix this by just checking for overflow in the delta live heap calculation
and if it would overflow, pick a small delta live heap. This probably
means that no sweeping will happen at all, but that's OK. This is a
transient state and the runtime will recover as soon as heapLive
increases again.

Note that deductSweepCredit doesn't check overflow on other operations
but that's OK: those operations are signed and extremely unlikely to
overflow. The subtraction targeted by this CL is only a problem because
it's unsigned. An alternative fix would be to make the operation signed,
but being explicit about the overflow situation seems worthwhile.

For #57523.
Fixes #58535.

Change-Id: Ib18f71f53468e913548aac6e5358830c72ef0215
Reviewed-on: https://go-review.googlesource.com/c/go/+/468376
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/runtime/mgcsweep.go b/src/runtime/mgcsweep.go
index 5f8f024b829f3ac70369c80f706fa51476b627f9..eb28d98ee4ddaf40532e5cf6d05d83dce621bf39 100644 (file)
--- a/src/runtime/mgcsweep.go
+++ b/src/runtime/mgcsweep.go
@@ -815,11 +815,30 @@ func deductSweepCredit(spanBytes uintptr, callerSweepPages uintptr) {
                traceGCSweepStart()
        }
 
+       // Fix debt if necessary.
 retry:
        sweptBasis := mheap_.pagesSweptBasis.Load()
-
-       // Fix debt if necessary.
-       newHeapLive := uintptr(atomic.Load64(&gcController.heapLive)-mheap_.sweepHeapLiveBasis) + spanBytes
+       live := atomic.Load64(&gcController.heapLive)
+       liveBasis := mheap_.sweepHeapLiveBasis
+       newHeapLive := spanBytes
+       if liveBasis < live {
+               // Only do this subtraction when we don't overflow. Otherwise, pagesTarget
+               // might be computed as something really huge, causing us to get stuck
+               // sweeping here until the next mark phase.
+               //
+               // Overflow can happen here if gcPaceSweeper is called concurrently with
+               // sweeping (i.e. not during a STW, like it usually is) because this code
+               // is intentionally racy. A concurrent call to gcPaceSweeper can happen
+               // if a GC tuning parameter is modified and we read an older value of
+               // heapLive than what was used to set the basis.
+               //
+               // This state should be transient, so it's fine to just let newHeapLive
+               // be a relatively small number. We'll probably just skip this attempt to
+               // sweep.
+               //
+               // See issue #57523.
+               newHeapLive += uintptr(live - liveBasis)
+       }
        pagesTarget := int64(mheap_.sweepPagesPerByte*float64(newHeapLive)) - int64(callerSweepPages)
        for pagesTarget > int64(mheap_.pagesSwept.Load()-sweptBasis) {
                if sweepone() == ^uintptr(0) {