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 3c3e110..62b7730 100644
--- a/src/runtime/export_test.go
+++ b/src/runtime/export_test.go
@@ -545,18 +545,23 @@
}
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 @@
// 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 @@
// 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 7895e48..0cb21f7 100644
--- a/src/runtime/mcache.go
+++ b/src/runtime/mcache.go
@@ -83,10 +83,13 @@
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 9eaacd9..823b556 100644
--- a/src/runtime/mgc.go
+++ b/src/runtime/mgc.go
@@ -216,16 +216,19 @@
//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 @@
gcBgMarkStartWorkers()
- gcResetMarkState()
+ systemstack(gcResetMarkState)
work.stwprocs, work.maxprocs = gomaxprocs, gomaxprocs
if work.stwprocs > ncpu {
@@ -2078,6 +2081,9 @@
}
}
+// 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 @@
//
// 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 3297c28..af2818a 100644
--- a/src/runtime/mheap.go
+++ b/src/runtime/mheap.go
@@ -29,6 +29,8 @@
//
//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 @@
// 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 @@
// 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 9250865..421580e 100644
--- a/src/runtime/mstats.go
+++ b/src/runtime/mstats.go
@@ -470,6 +470,9 @@
})
}
+// 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.
