From 32fef9908a6dcd523ae44766717662764e6c14ee Mon Sep 17 00:00:00 2001 From: Dmitriy Vyukov Date: Thu, 11 Jul 2013 15:57:36 -0400 Subject: [PATCH] =?utf8?q?runtime:=20fix=20CPU=20underutilization=20runtim?= =?utf8?q?e.newproc/ready=20are=20deliberately=20sloppy=20about=20waking?= =?utf8?q?=20new=20M's,=20they=20only=20ensure=20that=20there=20is=20at=20?= =?utf8?q?least=201=20spinning=20M.=20Currently=20to=20compensate=20for=20?= =?utf8?q?that,=20schedule()=20checks=20if=20the=20current=20P=20has=20loc?= =?utf8?q?al=20work=20and=20there=20are=20no=20spinning=20M's,=20it=20wake?= =?utf8?q?s=20up=20another=20one.=20It=20does=20not=20work=20if=20goroutin?= =?utf8?q?es=20do=20not=20call=20schedule.=20With=20this=20change=20a=20sp?= =?utf8?q?inning=20M=20wakes=20up=20another=20M=20when=20it=20finds=20work?= =?utf8?q?=20to=20do.=20It's=20also=20not=20ideal,=20but=20it=20fixes=20th?= =?utf8?q?e=20underutilization.=20A=20proper=20check=20would=20require=20t?= =?utf8?q?o=20know=20the=20exact=20number=20of=20runnable=20G's,=20but=20i?= =?utf8?q?t's=20too=20expensive=20to=20maintain.=20Fixes=20#5586.=20This?= =?utf8?q?=20is=20reincarnation=20of=20cl/9776044=20with=20the=20bug=20fix?= =?utf8?q?ed.=20The=20bug=20was=20due=20to=20code=20added=20after=20cl/977?= =?utf8?q?6044=20was=20created:=20if(tick=20-=20(((uint64)tick*0x4325c53fu?= =?utf8?q?)>>36)*61=20=3D=3D=200=20&&=20runtime=C2=B7sched.runqsize=20>=20?= =?utf8?q?0)=20{=20=20=20=20=20=20=20=20=20runtime=C2=B7lock(&runtime?= =?utf8?q?=C2=B7sched);=20=20=20=20=20=20=20=20=20gp=20=3D=20globrunqget(m?= =?utf8?q?->p,=201);=20=20=20=20=20=20=20=20=20runtime=C2=B7unlock(&runtim?= =?utf8?q?e=C2=B7sched);=20}=20If=20M=20gets=20gp=20from=20global=20runq?= =?utf8?q?=20here,=20it=20does=20not=20reset=20m->spinning.?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit R=golang-dev, rsc CC=golang-dev https://golang.org/cl/10743044 --- src/pkg/runtime/proc.c | 39 ++++++++++++++++++++++++------------ src/pkg/runtime/proc_test.go | 24 ++++++++++++++++++++++ 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/pkg/runtime/proc.c b/src/pkg/runtime/proc.c index cddbefc0f4..44741a66e8 100644 --- a/src/pkg/runtime/proc.c +++ b/src/pkg/runtime/proc.c @@ -1133,6 +1133,25 @@ stop: goto top; } +static void +resetspinning(void) +{ + int32 nmspinning; + + if(m->spinning) { + m->spinning = false; + nmspinning = runtime·xadd(&runtime·sched.nmspinning, -1); + if(nmspinning < 0) + runtime·throw("findrunnable: negative nmspinning"); + } else + nmspinning = runtime·atomicload(&runtime·sched.nmspinning); + + // M wakeup policy is deliberately somewhat conservative (see nmspinning handling), + // so see if we need to wakeup another P here. + if (nmspinning == 0 && runtime·atomicload(&runtime·sched.npidle) > 0) + wakep(); +} + // Injects the list of runnable G's into the scheduler. // Can run concurrently with GC. static void @@ -1184,28 +1203,22 @@ top: runtime·lock(&runtime·sched); gp = globrunqget(m->p, 1); runtime·unlock(&runtime·sched); + if(gp) + resetspinning(); } if(gp == nil) { gp = runqget(m->p); if(gp && m->spinning) runtime·throw("schedule: spinning with local work"); } - if(gp == nil) - gp = findrunnable(); - - if(m->spinning) { - m->spinning = false; - runtime·xadd(&runtime·sched.nmspinning, -1); + if(gp == nil) { + gp = findrunnable(); // blocks until work is available + resetspinning(); } - // M wakeup policy is deliberately somewhat conservative (see nmspinning handling), - // so see if we need to wakeup another M here. - if (m->p->runqhead != m->p->runqtail && - runtime·atomicload(&runtime·sched.nmspinning) == 0 && - runtime·atomicload(&runtime·sched.npidle) > 0) // TODO: fast atomic - wakep(); - if(gp->lockedm) { + // Hands off own p to the locked m, + // then blocks waiting for a new p. startlockedm(gp); goto top; } diff --git a/src/pkg/runtime/proc_test.go b/src/pkg/runtime/proc_test.go index 7e551d2fd1..8f3b407375 100644 --- a/src/pkg/runtime/proc_test.go +++ b/src/pkg/runtime/proc_test.go @@ -93,6 +93,30 @@ func TestYieldLocked(t *testing.T) { <-c } +func TestGoroutineParallelism(t *testing.T) { + const P = 4 + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(P)) + for try := 0; try < 10; try++ { + done := make(chan bool) + x := uint32(0) + for p := 0; p < P; p++ { + // Test that all P goroutines are scheduled at the same time + go func(p int) { + for i := 0; i < 3; i++ { + expected := uint32(P*i + p) + for atomic.LoadUint32(&x) != expected { + } + atomic.StoreUint32(&x, expected+1) + } + done <- true + }(p) + } + for p := 0; p < P; p++ { + <-done + } + } +} + func TestBlockLocked(t *testing.T) { const N = 10 c := make(chan bool) -- 2.48.1