From 8cc280aa727bc7159adfdd083861472aa3066a35 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Thu, 23 Jul 2020 20:13:49 +0000 Subject: [PATCH] runtime: define and enforce synchronization on heap_scan Currently heap_scan is mostly protected by the heap lock, but gcControllerState.revise sometimes accesses it without a lock. In an effort to make gcControllerState.revise callable from more contexts (and have its synchronization guarantees actually respected), make heap_scan atomically read from and written to, unless the world is stopped. Note that we don't update gcControllerState.revise's erroneous doc comment here because this change isn't about revise's guarantees, just about heap_scan. The comment is updated in a later change. Change-Id: Iddbbeb954767c704c2bd1d221f36e6c4fc9948a6 Reviewed-on: https://go-review.googlesource.com/c/go/+/246960 Run-TryBot: Michael Knyszek TryBot-Result: Go Bot Trust: Emmanuel Odeke Reviewed-by: Michael Pratt --- src/runtime/mgc.go | 5 +++-- src/runtime/mheap.go | 4 ++-- src/runtime/mstats.go | 4 +++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go index c42c7fbd29..94539dd770 100644 --- a/src/runtime/mgc.go +++ b/src/runtime/mgc.go @@ -494,6 +494,7 @@ func (c *gcControllerState) revise() { gcpercent = 100000 } live := atomic.Load64(&memstats.heap_live) + scan := atomic.Load64(&memstats.heap_scan) // Assume we're under the soft goal. Pace GC to complete at // next_gc assuming the heap is in steady-state. @@ -508,7 +509,7 @@ func (c *gcControllerState) revise() { // // (This is a float calculation to avoid overflowing on // 100*heap_scan.) - scanWorkExpected := int64(float64(memstats.heap_scan) * 100 / float64(100+gcpercent)) + scanWorkExpected := int64(float64(scan) * 100 / float64(100+gcpercent)) if live > memstats.next_gc || c.scanWork > scanWorkExpected { // We're past the soft goal, or we've already done more scan @@ -518,7 +519,7 @@ func (c *gcControllerState) revise() { heapGoal = int64(float64(memstats.next_gc) * maxOvershoot) // Compute the upper bound on the scan work remaining. - scanWorkExpected = int64(memstats.heap_scan) + scanWorkExpected = int64(scan) } // Compute the remaining scan work estimate. diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go index 1a57bcd66e..124bbacd1d 100644 --- a/src/runtime/mheap.go +++ b/src/runtime/mheap.go @@ -1168,7 +1168,7 @@ func (h *mheap) allocSpan(npages uintptr, manual bool, spanclass spanClass, sysS throw("mheap.allocSpan called with no P") } } - memstats.heap_scan += uint64(c.local_scan) + atomic.Xadd64(&memstats.heap_scan, int64(c.local_scan)) c.local_scan = 0 memstats.tinyallocs += uint64(c.local_tinyallocs) c.local_tinyallocs = 0 @@ -1375,7 +1375,7 @@ func (h *mheap) freeSpan(s *mspan) { systemstack(func() { c := getg().m.p.ptr().mcache lock(&h.lock) - memstats.heap_scan += uint64(c.local_scan) + atomic.Xadd64(&memstats.heap_scan, int64(c.local_scan)) c.local_scan = 0 memstats.tinyallocs += uint64(c.local_tinyallocs) c.local_tinyallocs = 0 diff --git a/src/runtime/mstats.go b/src/runtime/mstats.go index b95b332134..2c217ecf84 100644 --- a/src/runtime/mstats.go +++ b/src/runtime/mstats.go @@ -139,6 +139,8 @@ type mstats struct { // no-scan objects and no-scan tails of objects. // // Whenever this is updated, call gcController.revise(). + // + // Read and written atomically or with the world stopped. heap_scan uint64 // heap_marked is the number of bytes marked by the previous @@ -635,7 +637,7 @@ func flushallmcaches() { func purgecachedstats(c *mcache) { // Protected by either heap or GC lock. h := &mheap_ - memstats.heap_scan += uint64(c.local_scan) + atomic.Xadd64(&memstats.heap_scan, int64(c.local_scan)) c.local_scan = 0 memstats.tinyallocs += uint64(c.local_tinyallocs) c.local_tinyallocs = 0 -- 2.51.0