runtime: ensure mheap lock stack growth invariant is maintained

Currently there's an invariant in the runtime wherein the heap lock
can only be acquired on the system stack, otherwise a self-deadlock
could occur if the stack grows while the lock is held.

This invariant is upheld and documented in a number of situations (e.g.
allocManual, freeManual) but there are other places where the invariant
is either not maintained at all which risks self-deadlock (e.g.
setGCPercent, gcResetMarkState, allocmcache) or is maintained but
undocumented (e.g. gcSweep, readGCStats_m).

This change adds go:systemstack to any function that acquires the heap
lock or adds a systemstack(func() { ... }) around the critical section,
where appropriate. It also documents the invariant on (*mheap).lock
directly and updates repetitive documentation to refer to that comment.

Fixes #32105.

Change-Id: I702b1290709c118b837389c78efde25c51a2cafb
Reviewed-on: https://go-review.googlesource.com/c/go/+/177857
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Austin Clements <austin@google.com>

diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go
index 3c3e110f898f0c19e4b98654d80056d22eff70f1..62b7730c44c6a9c28452c2e152692eeac6cbe9fd 100644 (file)
--- a/src/runtime/export_test.go
+++ b/src/runtime/export_test.go
@@ -545,18 +545,23 @@ type Span struct {
 }
 
 func AllocSpan(base, npages uintptr, scavenged bool) Span {
-       lock(&mheap_.lock)
-       s := (*mspan)(mheap_.spanalloc.alloc())
-       unlock(&mheap_.lock)
+       var s *mspan
+       systemstack(func() {
+               lock(&mheap_.lock)
+               s = (*mspan)(mheap_.spanalloc.alloc())
+               unlock(&mheap_.lock)
+       })
        s.init(base, npages)
        s.scavenged = scavenged
        return Span{s}
 }
 
 func (s *Span) Free() {
-       lock(&mheap_.lock)
-       mheap_.spanalloc.free(unsafe.Pointer(s.mspan))
-       unlock(&mheap_.lock)
+       systemstack(func() {
+               lock(&mheap_.lock)
+               mheap_.spanalloc.free(unsafe.Pointer(s.mspan))
+               unlock(&mheap_.lock)
+       })
        s.mspan = nil
 }
 
@@ -629,9 +634,11 @@ func (t *Treap) Insert(s Span) {
        // allocation which requires the mheap_ lock to manipulate.
        // Locking here is safe because the treap itself never allocs
        // or otherwise ends up grabbing this lock.
-       lock(&mheap_.lock)
-       t.insert(s.mspan)
-       unlock(&mheap_.lock)
+       systemstack(func() {
+               lock(&mheap_.lock)
+               t.insert(s.mspan)
+               unlock(&mheap_.lock)
+       })
        t.CheckInvariants()
 }
 
@@ -644,17 +651,21 @@ func (t *Treap) Erase(i TreapIter) {
        // freeing which requires the mheap_ lock to manipulate.
        // Locking here is safe because the treap itself never allocs
        // or otherwise ends up grabbing this lock.
-       lock(&mheap_.lock)
-       t.erase(i.treapIter)
-       unlock(&mheap_.lock)
+       systemstack(func() {
+               lock(&mheap_.lock)
+               t.erase(i.treapIter)
+               unlock(&mheap_.lock)
+       })
        t.CheckInvariants()
 }
 
 func (t *Treap) RemoveSpan(s Span) {
        // See Erase about locking.
-       lock(&mheap_.lock)
-       t.removeSpan(s.mspan)
-       unlock(&mheap_.lock)
+       systemstack(func() {
+               lock(&mheap_.lock)
+               t.removeSpan(s.mspan)
+               unlock(&mheap_.lock)
+       })
        t.CheckInvariants()
 }
 

diff --git a/src/runtime/mcache.go b/src/runtime/mcache.go
index 7895e489bccae90e0f5ac17c2c0ba6b67a5308d6..0cb21f719049b52e51d00b3e0ca7aa8b66d07c88 100644 (file)
--- a/src/runtime/mcache.go
+++ b/src/runtime/mcache.go
@@ -83,10 +83,13 @@ type stackfreelist struct {
 var emptymspan mspan
 
 func allocmcache() *mcache {
-       lock(&mheap_.lock)
-       c := (*mcache)(mheap_.cachealloc.alloc())
-       c.flushGen = mheap_.sweepgen
-       unlock(&mheap_.lock)
+       var c *mcache
+       systemstack(func() {
+               lock(&mheap_.lock)
+               c = (*mcache)(mheap_.cachealloc.alloc())
+               c.flushGen = mheap_.sweepgen
+               unlock(&mheap_.lock)
+       })
        for i := range c.alloc {
                c.alloc[i] = &emptymspan
        }

diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go
index 9eaacd933d74661eae9b8e010b7152aba5e2a4b0..823b556e533ee9bb253ddc81bf46d16d5c15af77 100644 (file)
--- a/src/runtime/mgc.go
+++ b/src/runtime/mgc.go
@@ -216,16 +216,19 @@ func gcenable() {
 
 //go:linkname setGCPercent runtime/debug.setGCPercent
 func setGCPercent(in int32) (out int32) {
-       lock(&mheap_.lock)
-       out = gcpercent
-       if in < 0 {
-               in = -1
-       }
-       gcpercent = in
-       heapminimum = defaultHeapMinimum * uint64(gcpercent) / 100
-       // Update pacing in response to gcpercent change.
-       gcSetTriggerRatio(memstats.triggerRatio)
-       unlock(&mheap_.lock)
+       // Run on the system stack since we grab the heap lock.
+       systemstack(func() {
+               lock(&mheap_.lock)
+               out = gcpercent
+               if in < 0 {
+                       in = -1
+               }
+               gcpercent = in
+               heapminimum = defaultHeapMinimum * uint64(gcpercent) / 100
+               // Update pacing in response to gcpercent change.
+               gcSetTriggerRatio(memstats.triggerRatio)
+               unlock(&mheap_.lock)
+       })
 
        // If we just disabled GC, wait for any concurrent GC mark to
        // finish so we always return with no GC running.
@@ -1261,7 +1264,7 @@ func gcStart(trigger gcTrigger) {
 
        gcBgMarkStartWorkers()
 
-       gcResetMarkState()
+       systemstack(gcResetMarkState)
 
        work.stwprocs, work.maxprocs = gomaxprocs, gomaxprocs
        if work.stwprocs > ncpu {
@@ -2078,6 +2081,9 @@ func gcMark(start_time int64) {
        }
 }
 
+// gcSweep must be called on the system stack because it acquires the heap
+// lock. See mheap for details.
+//go:systemstack
 func gcSweep(mode gcMode) {
        if gcphase != _GCoff {
                throw("gcSweep being done but phase is not GCoff")
@@ -2134,6 +2140,11 @@ func gcSweep(mode gcMode) {
 //
 // This is safe to do without the world stopped because any Gs created
 // during or after this will start out in the reset state.
+//
+// gcResetMarkState must be called on the system stack because it acquires
+// the heap lock. See mheap for details.
+//
+//go:systemstack
 func gcResetMarkState() {
        // This may be called during a concurrent phase, so make sure
        // allgs doesn't change.

diff --git a/src/runtime/mheap.go b/src/runtime/mheap.go
index 3297c287d4c6ea8d96337ddbbbf3a5a9168977fe..af2818a2bdf8dec8494dd71ff77ffefafd7ed239 100644 (file)
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -29,6 +29,8 @@ const minPhysPageSize = 4096
 //
 //go:notinheap
 type mheap struct {
+       // lock must only be acquired on the system stack, otherwise a g
+       // could self-deadlock if its stack grows with the lock held.
        lock      mutex
        free      mTreap // free spans
        sweepgen  uint32 // sweep generation, see comment in mspan
@@ -1095,9 +1097,8 @@ func (h *mheap) alloc(npage uintptr, spanclass spanClass, large bool, needzero b
 // The memory backing the returned span may not be zeroed if
 // span.needzero is set.
 //
-// allocManual must be called on the system stack to prevent stack
-// growth. Since this is used by the stack allocator, stack growth
-// during allocManual would self-deadlock.
+// allocManual must be called on the system stack because it acquires
+// the heap lock. See mheap for details.
 //
 //go:systemstack
 func (h *mheap) allocManual(npage uintptr, stat *uint64) *mspan {
@@ -1303,8 +1304,8 @@ func (h *mheap) freeSpan(s *mspan, large bool) {
 // This must only be called when gcphase == _GCoff. See mSpanState for
 // an explanation.
 //
-// freeManual must be called on the system stack to prevent stack
-// growth, just like allocManual.
+// freeManual must be called on the system stack because it acquires
+// the heap lock. See mheap for details.
 //
 //go:systemstack
 func (h *mheap) freeManual(s *mspan, stat *uint64) {

diff --git a/src/runtime/mstats.go b/src/runtime/mstats.go
index 9250865ed180e017176683b244fc73d267706b54..421580eec3a709f3a28bbe227209ae2b3e3368b5 100644 (file)
--- a/src/runtime/mstats.go
+++ b/src/runtime/mstats.go
@@ -470,6 +470,9 @@ func readGCStats(pauses *[]uint64) {
        })
 }
 
+// readGCStats_m must be called on the system stack because it acquires the heap
+// lock. See mheap for details.
+//go:systemstack
 func readGCStats_m(pauses *[]uint64) {
        p := *pauses
        // Calling code in runtime/debug should make the slice large enough.