]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.13] runtime: fix lock acquire cycles related to scavenge.lock
authorMichael Anthony Knyszek <mknyszek@google.com>
Thu, 5 Sep 2019 16:34:00 +0000 (16:34 +0000)
committerAndrew Bonventre <andybons@golang.org>
Wed, 2 Oct 2019 19:51:10 +0000 (19:51 +0000)
There are currently two edges in the lock cycle graph caused by
scavenge.lock: with sched.lock and mheap_.lock. These edges appear
because of the call to ready() and stack growths respectively.
Furthermore, there's already an invariant in the code wherein
mheap_.lock must be acquired before scavenge.lock, hence the cycle.

The fix to this is to bring scavenge.lock higher in the lock cycle
graph, such that sched.lock and mheap_.lock are only acquired once
scavenge.lock is already held.

To faciliate this change, we move scavenger waking outside of
gcSetTriggerRatio such that it doesn't have to happen with the heap
locked. Furthermore, we check scavenge generation numbers with the heap
locked by using gopark instead of goparkunlock, and specify a function
which aborts the park should there be any skew in generation count.

Fixes #34150.

Change-Id: I3519119214bac66375e2b1262b36ce376c820d12
Reviewed-on: https://go-review.googlesource.com/c/go/+/191977
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit 62e415655238a3c0103c1b70e6805edf8193c543)
Reviewed-on: https://go-review.googlesource.com/c/go/+/197501
Reviewed-by: Austin Clements <austin@google.com>
src/runtime/mgc.go
src/runtime/mgcscavenge.go
src/runtime/mheap.go

index 823b556e533ee9bb253ddc81bf46d16d5c15af77..8ad2198e9f41d2312a2981cab225c93a8b63c1e8 100644 (file)
@@ -229,6 +229,8 @@ func setGCPercent(in int32) (out int32) {
                gcSetTriggerRatio(memstats.triggerRatio)
                unlock(&mheap_.lock)
        })
+       // Pacing changed, so the scavenger should be awoken.
+       wakeScavenger()
 
        // If we just disabled GC, wait for any concurrent GC mark to
        // finish so we always return with no GC running.
@@ -1652,6 +1654,9 @@ func gcMarkTermination(nextTriggerRatio float64) {
        // Update GC trigger and pacing for the next cycle.
        gcSetTriggerRatio(nextTriggerRatio)
 
+       // Pacing changed, so the scavenger should be awoken.
+       wakeScavenger()
+
        // Update timing memstats
        now := nanotime()
        sec, nsec, _ := time_now()
index 45a9eb2b2aa9e208ee75d27986dc856a5e64fd39..e9010eb98a0808e718a64666688949d7ae15c380 100644 (file)
@@ -154,36 +154,41 @@ func gcPaceScavenger() {
 
        now := nanotime()
 
-       lock(&scavenge.lock)
-
        // Update all the pacing parameters in mheap with scavenge.lock held,
        // so that scavenge.gen is kept in sync with the updated values.
        mheap_.scavengeRetainedGoal = retainedGoal
        mheap_.scavengeRetainedBasis = retainedNow
        mheap_.scavengeTimeBasis = now
        mheap_.scavengeBytesPerNS = float64(totalWork) / float64(totalTime)
-       scavenge.gen++ // increase scavenge generation
-
-       // Wake up background scavenger if needed, since the pacing was just updated.
-       wakeScavengerLocked()
-
-       unlock(&scavenge.lock)
+       mheap_.scavengeGen++ // increase scavenge generation
 }
 
-// State of the background scavenger.
+// Sleep/wait state of the background scavenger.
 var scavenge struct {
        lock   mutex
        g      *g
        parked bool
        timer  *timer
-       gen    uint32 // read with either lock or mheap_.lock, write with both
+
+       // Generation counter.
+       //
+       // It represents the last generation count (as defined by
+       // mheap_.scavengeGen) checked by the scavenger and is updated
+       // each time the scavenger checks whether it is on-pace.
+       //
+       // Skew between this field and mheap_.scavengeGen is used to
+       // determine whether a new update is available.
+       //
+       // Protected by mheap_.lock.
+       gen uint64
 }
 
-// wakeScavengerLocked unparks the scavenger if necessary. It must be called
+// wakeScavenger unparks the scavenger if necessary. It must be called
 // after any pacing update.
 //
-// scavenge.lock must be held.
-func wakeScavengerLocked() {
+// mheap_.lock and scavenge.lock must not be held.
+func wakeScavenger() {
+       lock(&scavenge.lock)
        if scavenge.parked {
                // Try to stop the timer but we don't really care if we succeed.
                // It's possible that either a timer was never started, or that
@@ -198,11 +203,10 @@ func wakeScavengerLocked() {
                scavenge.parked = false
                ready(scavenge.g, 0, true)
        }
+       unlock(&scavenge.lock)
 }
 
 // scavengeSleep attempts to put the scavenger to sleep for ns.
-// It also checks to see if gen != scavenge.gen before going to sleep,
-// and aborts if true (meaning an update had occurred).
 //
 // Note that this function should only be called by the scavenger.
 //
@@ -210,24 +214,32 @@ func wakeScavengerLocked() {
 // to sleep at all if there's a pending pacing change.
 //
 // Returns false if awoken early (i.e. true means a complete sleep).
-func scavengeSleep(gen uint32, ns int64) bool {
+func scavengeSleep(ns int64) bool {
        lock(&scavenge.lock)
 
-       // If there was an update, just abort the sleep.
-       if scavenge.gen != gen {
+       // First check if there's a pending update.
+       // If there is one, don't bother sleeping.
+       var hasUpdate bool
+       systemstack(func() {
+               lock(&mheap_.lock)
+               hasUpdate = mheap_.scavengeGen != scavenge.gen
+               unlock(&mheap_.lock)
+       })
+       if hasUpdate {
                unlock(&scavenge.lock)
                return false
        }
 
        // Set the timer.
+       //
+       // This must happen here instead of inside gopark
+       // because we can't close over any variables without
+       // failing escape analysis.
        now := nanotime()
        scavenge.timer.when = now + ns
        startTimer(scavenge.timer)
 
-       // Park the goroutine. It's fine that we don't publish the
-       // fact that the timer was set; even if the timer wakes up
-       // and fire scavengeReady before we park, it'll block on
-       // scavenge.lock.
+       // Mark ourself as asleep and go to sleep.
        scavenge.parked = true
        goparkunlock(&scavenge.lock, waitReasonSleep, traceEvGoSleep, 2)
 
@@ -248,9 +260,7 @@ func bgscavenge(c chan int) {
 
        scavenge.timer = new(timer)
        scavenge.timer.f = func(_ interface{}, _ uintptr) {
-               lock(&scavenge.lock)
-               wakeScavengerLocked()
-               unlock(&scavenge.lock)
+               wakeScavenger()
        }
 
        c <- 1
@@ -279,14 +289,14 @@ func bgscavenge(c chan int) {
                released := uintptr(0)
                park := false
                ttnext := int64(0)
-               gen := uint32(0)
 
                // Run on the system stack since we grab the heap lock,
                // and a stack growth with the heap lock means a deadlock.
                systemstack(func() {
                        lock(&mheap_.lock)
 
-                       gen = scavenge.gen
+                       // Update the last generation count that the scavenger has handled.
+                       scavenge.gen = mheap_.scavengeGen
 
                        // If background scavenging is disabled or if there's no work to do just park.
                        retained := heapRetained()
@@ -343,7 +353,7 @@ func bgscavenge(c chan int) {
                if released == 0 {
                        // If we were unable to release anything this may be because there's
                        // no free memory available to scavenge. Go to sleep and try again.
-                       if scavengeSleep(gen, retryDelayNS) {
+                       if scavengeSleep(retryDelayNS) {
                                // If we successfully slept through the delay, back off exponentially.
                                retryDelayNS *= 2
                        }
@@ -355,7 +365,7 @@ func bgscavenge(c chan int) {
                        // If there's an appreciable amount of time until the next scavenging
                        // goal, just sleep. We'll get woken up if anything changes and this
                        // way we avoid spinning.
-                       scavengeSleep(gen, ttnext)
+                       scavengeSleep(ttnext)
                        continue
                }
 
index 706603aba4e87bcc6bf5c0fca0baee27cb4c9ce0..e835fc137fbed2b455b34a616cf6b091cce81fba 100644 (file)
@@ -107,6 +107,7 @@ type mheap struct {
        scavengeRetainedBasis uint64
        scavengeBytesPerNS    float64
        scavengeRetainedGoal  uint64
+       scavengeGen           uint64 // incremented on each pacing update
 
        // Page reclaimer state