code review 5307052: runtime: include bootstrap m in mcpu accounting

runtime: include bootstrap m in mcpu accounting

[hector, 2011-10-23 17:23:30]

Hello rsc@golang.org, dvyukov@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[hector, 2011-10-23 17:28:04]

Fixes the issue here:
http://groups.google.com/group/golang-nuts/browse_thread/thread/172eba23fb81f...

[dvyukov, 2011-10-24 06:47:37]

An M can't run w/o incrementing mcpu, otherwise GC starts w/o waiting for it
(now even bootstrap M can't do that). mcpu can't be incremented w/o incrementing
grunning (otherwise spurious deadlock report). It is the second issue with the
logic. I think we must reflect the logic in schedule().
If you agree, please make mcpu-- and grunning-- unconditional in schedule(), do
not set helpgc for bootstrap M and gchelp M's. That will eliminate
"if(m->helpgc)" branch. I think it will make the logic clearer. I would also add
underflow check for grunning--.

[hector, 2011-10-24 08:12:33]

On 24 October 2011 07:47,  <dvyukov@google.com> wrote:
> If you agree, please make mcpu-- and grunning-- unconditional in
> schedule().

Do we want to do this even if mcpu++ happens before mnextg (as happens
in matchmg)?  Probably ok for an M waiting on global queue, but what
about new M started by startm?

[dvyukov, 2011-10-24 08:23:38]

On 2011/10/24 08:12:33, hector wrote:
> On 24 October 2011 07:47,  <mailto:dvyukov@google.com> wrote:
> > If you agree, please make mcpu-- and grunning-- unconditional in
> > schedule().
> 
> Do we want to do this even if mcpu++ happens before mnextg (as happens
> in matchmg)?  Probably ok for an M waiting on global queue, but what
> about new M started by startm?

What do you mean by "new M started by startm"? startm is called either from
matchmg (where we must do mcpu++/grunning++) of starttheworld (where we now do
mcpu++/grunning++). There is also bootstrap M, where we now do mcpu++/grunning++
as well. Are there any other Ms (other callers of startm)?
If there is a case when schedule does not do mcpu--/grunning--, it is most
likely a bug (like it is with bootstrap M).

[hector, 2011-10-24 08:45:05]

On 2011/10/24 08:23:38, dvyukov wrote:
> On 2011/10/24 08:12:33, hector wrote:
> > On 24 October 2011 07:47,  <mailto:dvyukov@google.com> wrote:
> > > If you agree, please make mcpu-- and grunning-- unconditional in
> > > schedule().
> > 
> > Do we want to do this even if mcpu++ happens before mnextg (as happens
> > in matchmg)?  Probably ok for an M waiting on global queue, but what
> > about new M started by startm?
> 
> What do you mean by "new M started by startm"? startm is called either from
> matchmg (where we must do mcpu++/grunning++) of starttheworld (where we now do
> mcpu++/grunning++). There is also bootstrap M, where we now do
mcpu++/grunning++
> as well. Are there any other Ms (other callers of startm)?
> If there is a case when schedule does not do mcpu--/grunning--, it is most
> likely a bug (like it is with bootstrap M).

Sorry, I meant that the new M will enter scheduler with mcpu already incremented
and m->nextg set, so I'm not sure if it's ok to decrement mcpu unconditionally. 
Because in some places the mcpu check happens without schedlock.

[dvyukov, 2011-10-24 09:57:53]

On 2011/10/24 08:45:05, hector wrote:
> On 2011/10/24 08:23:38, dvyukov wrote:
> > On 2011/10/24 08:12:33, hector wrote:
> > > On 24 October 2011 07:47,  <mailto:dvyukov@google.com> wrote:
> > > > If you agree, please make mcpu-- and grunning-- unconditional in
> > > > schedule().
> > > 
> > > Do we want to do this even if mcpu++ happens before mnextg (as happens
> > > in matchmg)?  Probably ok for an M waiting on global queue, but what
> > > about new M started by startm?
> > 
> > What do you mean by "new M started by startm"? startm is called either from
> > matchmg (where we must do mcpu++/grunning++) of starttheworld (where we now
do
> > mcpu++/grunning++). There is also bootstrap M, where we now do
> mcpu++/grunning++
> > as well. Are there any other Ms (other callers of startm)?
> > If there is a case when schedule does not do mcpu--/grunning--, it is most
> > likely a bug (like it is with bootstrap M).
> 
> Sorry, I meant that the new M will enter scheduler with mcpu already
incremented
> and m->nextg set, so I'm not sure if it's ok to decrement mcpu
unconditionally. 
> Because in some places the mcpu check happens without schedlock.


I see. Then we will have to canaddmcpu() in nextgandunlock() if m->nextg is set.
Yeah, it's better to not touch that :)
The problem is that the logic is quite fragile and implicit now. It's basically
impossible to understand all that ++ and --. Can't we then structure schedule()
as follows:

if(gp) {
  mpcu--;
  grunning--;
  // as before
} else if(m->gchelp) {
  mpcu--;
  grunning--;
} else if(m->nextg) {
  // no-op
} else {
  throw("can't happen");
}

It makes it clear that M can arrive to schedule() in only 3 states.

[hector, 2011-10-24 11:03:52]

Works ok for me.  I'll add it to the CL later today then.

[hector, 2011-10-24 18:52:20]

PTAL

[dvyukov, 2011-10-25 06:35:16]

On 2011/10/24 18:52:20, hector wrote:
> PTAL

LGTM
Just to make sure all.bash does not crash, right? :)

[hector, 2011-10-25 07:34:11]

On 25 October 2011 07:35,  <dvyukov@google.com> wrote:
> LGTM
> Just to make sure all.bash does not crash, right? :)

Tests run fine on Windows.  As for the rest that's what the buildbots
are for <ducks>

[hector, 2011-10-25 07:35:36]

*** Submitted as http://code.google.com/p/go/source/detail?r=e7977be1ca45 ***

runtime: include bootstrap m in mcpu accounting

R=rsc, dvyukov
CC=golang-dev
http://codereview.appspot.com/5307052