From 3c96ae08701dd3ed66e0f8a81a80e3336a4d9aae Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Sun, 5 May 2024 21:17:27 +0000 Subject: [PATCH] [release-branch.go1.22] runtime: update large object stats before freeSpan in sweep MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. For #67019. Fixes #67188. Change-Id: Ib02bd8dcd1cf8cd1bc0110b6141e74f678c10445 Reviewed-on: https://go-review.googlesource.com/c/go/+/583380 Auto-Submit: Michael Knyszek Reviewed-by: Felix Geisendörfer LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Pratt (cherry picked from commit 36d32f68f41561fb64677297e3733f5d5b866c2a) Reviewed-on: https://go-review.googlesource.com/c/go/+/584339 Reviewed-by: Carlos Amedee --- src/runtime/metrics_test.go | 35 +++++++++++++++++++++++++++++++++++ src/runtime/mgcsweep.go | 23 +++++++++++++---------- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/runtime/metrics_test.go b/src/runtime/metrics_test.go index d7f41334cd..5866107275 100644 --- a/src/runtime/metrics_test.go +++ b/src/runtime/metrics_test.go @@ -1290,3 +1290,38 @@ func (w *contentionWorker) run() { for w.fn() { } } + +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 i := 0; i < 10; i++ { + runtime.Escape(make([]byte, 1<<20)) + } + runtime.GC() + select { + case <-done: + return + default: + } + } + }() + s := []metrics.Sample{ + {Name: "/memory/classes/heap/unused:bytes"}, + } + for i := 0; i < 1000; i++ { + 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 3dbe9bcec7..35be794947 100644 --- a/src/runtime/mgcsweep.go +++ b/src/runtime/mgcsweep.go @@ -770,6 +770,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 @@ -802,16 +815,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 } -- 2.50.0