runtime: update large object stats before freeSpan in sweep

Currently freeSpan is called before large object stats are updated when
sweeping large objects. This means heapStats.inHeap might get subtracted
before the large object is added to the largeFree field. The end result
is that the /memory/classes/heap/unused:bytes metric, which subtracts
live objects (alloc-free) from inHeap may overflow.

Fix this by always updating the large object stats before calling
freeSpan.

Fixes #67019.

Change-Id: Ib02bd8dcd1cf8cd1bc0110b6141e74f678c10445
Reviewed-on: https://go-review.googlesource.com/c/go/+/583380
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Felix Geisendörfer <felix.geisendoerfer@datadoghq.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>

diff --git a/src/runtime/metrics_test.go b/src/runtime/metrics_test.go
index 859bc712f16e30990f53178af272c41a9934de60..cb8d17d15b66500b02f30e32d08f81173b155782 100644 (file)
--- a/src/runtime/metrics_test.go
+++ b/src/runtime/metrics_test.go
@@ -7,6 +7,7 @@ package runtime_test
 import (
        "bytes"
        "fmt"
+       "internal/abi"
        "internal/goexperiment"
        "internal/profile"
        "internal/testenv"
@@ -1331,3 +1332,38 @@ func TestCPUStats(t *testing.T) {
                t.Error("idle time is zero")
        }
 }
+
+func TestMetricHeapUnusedLargeObjectOverflow(t *testing.T) {
+       // This test makes sure /memory/classes/heap/unused:bytes
+       // doesn't overflow when allocating and deallocating large
+       // objects. It is a regression test for #67019.
+       done := make(chan struct{})
+       var wg sync.WaitGroup
+       wg.Add(1)
+       go func() {
+               defer wg.Done()
+               for {
+                       for range 10 {
+                               abi.Escape(make([]byte, 1<<20))
+                       }
+                       runtime.GC()
+                       select {
+                       case <-done:
+                               return
+                       default:
+                       }
+               }
+       }()
+       s := []metrics.Sample{
+               {Name: "/memory/classes/heap/unused:bytes"},
+       }
+       for range 1000 {
+               metrics.Read(s)
+               if s[0].Value.Uint64() > 1<<40 {
+                       t.Errorf("overflow")
+                       break
+               }
+       }
+       done <- struct{}{}
+       wg.Wait()
+}

diff --git a/src/runtime/mgcsweep.go b/src/runtime/mgcsweep.go
index ae252cd47ae8a1b5e95d0dd8096e8b6bea1750fb..f53330a5b976563764d1600b41a1b09853411ebc 100644 (file)
--- a/src/runtime/mgcsweep.go
+++ b/src/runtime/mgcsweep.go
@@ -785,6 +785,19 @@ func (sl *sweepLocked) sweep(preserve bool) bool {
                if nfreed != 0 {
                        // Free large object span to heap.
 
+                       // Count the free in the consistent, external stats.
+                       //
+                       // Do this before freeSpan, which might update heapStats' inHeap
+                       // value. If it does so, then metrics that subtract object footprint
+                       // from inHeap might overflow. See #67019.
+                       stats := memstats.heapStats.acquire()
+                       atomic.Xadd64(&stats.largeFreeCount, 1)
+                       atomic.Xadd64(&stats.largeFree, int64(size))
+                       memstats.heapStats.release()
+
+                       // Count the free in the inconsistent, internal stats.
+                       gcController.totalFree.Add(int64(size))
+
                        // NOTE(rsc,dvyukov): The original implementation of efence
                        // in CL 22060046 used sysFree instead of sysFault, so that
                        // the operating system would eventually give the memory
@@ -817,16 +830,6 @@ func (sl *sweepLocked) sweep(preserve bool) bool {
                                // invalid pointer. See arena.go:(*mheap).allocUserArenaChunk.
                                *(*uintptr)(unsafe.Pointer(&s.largeType)) = 0
                        }
-
-                       // Count the free in the consistent, external stats.
-                       stats := memstats.heapStats.acquire()
-                       atomic.Xadd64(&stats.largeFreeCount, 1)
-                       atomic.Xadd64(&stats.largeFree, int64(size))
-                       memstats.heapStats.release()
-
-                       // Count the free in the inconsistent, internal stats.
-                       gcController.totalFree.Add(int64(size))
-
                        return true
                }