From 0bae74e8c9b5fab3baf61bde0169f4aa5e287bdc Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Sun, 30 Oct 2016 20:43:53 -0400 Subject: [PATCH] runtime: wake idle Ps when enqueuing GC work If the scheduler has no user work and there's no GC work visible, it puts the P to sleep (or blocks on the network). However, if we later enqueue more GC work, there's currently nothing that specifically wakes up the scheduler to let it start an idle GC worker. As a result, we can underutilize the CPU during GC if Ps have been put to sleep. Fix this by making GC wake idle Ps when work buffers are put on the full list. We already have a hook to do this, since we use this to preempt a random P if we need more dedicated workers. We expand this hook to instead wake an idle P if there is one. The logic we use for this is identical to the logic used to wake an idle P when we ready a goroutine. To make this really sound, we also fix the scheduler to re-check the idle GC worker condition after releasing its P. This closes a race where 1) the scheduler checks for idle work and finds none, 2) new work is enqueued but there are no idle Ps so none are woken, and 3) the scheduler releases its P. There is one subtlety here. Currently we call enlistWorker directly from putfull, but the gcWork is in an inconsistent state in the places that call putfull. This isn't a problem right now because nothing that enlistWorker does touches the gcWork, but with the added call to wakep, it's possible to get a recursive call into the gcWork (specifically, while write barriers are disallowed, this can do an allocation, which can dispose a gcWork, which can put a workbuf). To handle this, we lift the enlistWorker calls up a layer and delay them until the gcWork is in a consistent state. Fixes #14179. Change-Id: Ia2467a52e54c9688c3c1752e1fc00f5b37bbfeeb Reviewed-on: https://go-review.googlesource.com/32434 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Dmitry Vyukov --- src/runtime/mgc.go | 8 ++++++++ src/runtime/mgcwork.go | 22 ++++++++++++++++------ src/runtime/proc.go | 20 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go index 58b52e8bed..f97b589839 100644 --- a/src/runtime/mgc.go +++ b/src/runtime/mgc.go @@ -624,6 +624,14 @@ func (c *gcControllerState) endCycle() { // //go:nowritebarrier func (c *gcControllerState) enlistWorker() { + // If there are idle Ps, wake one so it will run an idle worker. + if atomic.Load(&sched.npidle) != 0 && atomic.Load(&sched.nmspinning) == 0 { + wakep() + return + } + + // There are no idle Ps. If we need more dedicated workers, + // try to preempt a running P so it will switch to a worker. if c.dedicatedMarkWorkersNeeded <= 0 { return } diff --git a/src/runtime/mgcwork.go b/src/runtime/mgcwork.go index 699982e01d..5eb05a767c 100644 --- a/src/runtime/mgcwork.go +++ b/src/runtime/mgcwork.go @@ -99,6 +99,7 @@ func (w *gcWork) init() { // obj must point to the beginning of a heap object or an oblet. //go:nowritebarrier func (w *gcWork) put(obj uintptr) { + flushed := false wbuf := w.wbuf1.ptr() if wbuf == nil { w.init() @@ -111,11 +112,20 @@ func (w *gcWork) put(obj uintptr) { putfull(wbuf) wbuf = getempty() w.wbuf1 = wbufptrOf(wbuf) + flushed = true } } wbuf.obj[wbuf.nobj] = obj wbuf.nobj++ + + // If we put a buffer on full, let the GC controller know so + // it can encourage more workers to run. We delay this until + // the end of put so that w is in a consistent state, since + // enlistWorker may itself manipulate w. + if flushed && gcphase == _GCmark { + gcController.enlistWorker() + } } // putFast does a put and returns true if it can be done quickly @@ -263,6 +273,12 @@ func (w *gcWork) balance() { w.wbuf2 = wbufptrOf(getempty()) } else if wbuf := w.wbuf1.ptr(); wbuf.nobj > 4 { w.wbuf1 = wbufptrOf(handoff(wbuf)) + } else { + return + } + // We flushed a buffer to the full list, so wake a worker. + if gcphase == _GCmark { + gcController.enlistWorker() } } @@ -337,12 +353,6 @@ func putempty(b *workbuf) { func putfull(b *workbuf) { b.checknonempty() lfstackpush(&work.full, &b.node) - - // We just made more work available. Let the GC controller - // know so it can encourage more workers to run. - if gcphase == _GCmark { - gcController.enlistWorker() - } } // trygetfull tries to get a full or partially empty workbuffer. diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 1b21b37de8..cad1b1c0f4 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -2023,6 +2023,26 @@ stop: } } + // Check for idle-priority GC work again. + if gcBlackenEnabled != 0 && gcMarkWorkAvailable(nil) { + lock(&sched.lock) + _p_ = pidleget() + if _p_ != nil && _p_.gcBgMarkWorker == 0 { + pidleput(_p_) + _p_ = nil + } + unlock(&sched.lock) + if _p_ != nil { + acquirep(_p_) + if wasSpinning { + _g_.m.spinning = true + atomic.Xadd(&sched.nmspinning, 1) + } + // Go back to idle GC check. + goto stop + } + } + // poll network if netpollinited() && atomic.Xchg64(&sched.lastpoll, 0) != 0 { if _g_.m.p != 0 {