code review 10051045: runtime: fix scheduler race condition

runtime: fix scheduler race condition
In starttheworld() we assume that P's with local work
are situated in the beginning of idle P list.
However, once we start the first M, it can execute all local G's
and steal G's from other P's.
That breaks the assumption above. Thus starttheworld() will fail
to start some P's with local work.
It seems that it can not lead to very bad things, but still
it's wrong and breaks other assumtions
(e.g. we can have a spinning M with local work).
The fix is to collect all P's with local work first,
and only then start them.

[dvyukov, 2013-06-05 20:19:40]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[rsc, 2013-06-10 19:08:35]

https://codereview.appspot.com/10051045/diff/5001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/10051045/diff/5001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:444: add = false;
insert comment before this line, and swap to make comment easier:

// Start M to run P. Do not start another M below.
newm(nil, p);
add = false;

How does this avoid creating one M for every P that is pending?
Shouldn't it stop when reaching GOMAXPROCS?

[dvyukov, 2013-06-10 19:24:16]

https://codereview.appspot.com/10051045/diff/5001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/10051045/diff/5001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:444: add = false;
On 2013/06/10 19:08:35, rsc wrote:
> insert comment before this line, and swap to make comment easier:
> 
> // Start M to run P. Do not start another M below.
> newm(nil, p);
> add = false;
> 
> How does this avoid creating one M for every P that is pending?
> Shouldn't it stop when reaching GOMAXPROCS?

I do not understand the question.
It avoids creating excessive M's by doing "p->m = mget();" above under the lock.
There is exactly GOMAXPROCS P's, it does not create any new P's.

[dvyukov, 2013-06-10 19:26:25]

https://codereview.appspot.com/10051045/diff/5001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/10051045/diff/5001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:444: add = false;
On 2013/06/10 19:08:35, rsc wrote:
> insert comment before this line, and swap to make comment easier:
> 
> // Start M to run P. Do not start another M below.
> newm(nil, p);
> add = false;

Done

[rsc, 2013-06-10 20:25:59]

LGTM

Sorry, my mistake. The equivalent of the 'g' variable in the Go runtime is 'p'
in the Plan 9 kernel. So I was effectively reading all the p's as g's, hence my
confusion.

[dvyukov, 2013-06-12 14:46:46]

*** Submitted as https://code.google.com/p/go/source/detail?r=8ae475f7599b ***

runtime: fix scheduler race condition
In starttheworld() we assume that P's with local work
are situated in the beginning of idle P list.
However, once we start the first M, it can execute all local G's
and steal G's from other P's.
That breaks the assumption above. Thus starttheworld() will fail
to start some P's with local work.
It seems that it can not lead to very bad things, but still
it's wrong and breaks other assumtions
(e.g. we can have a spinning M with local work).
The fix is to collect all P's with local work first,
and only then start them.

R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/10051045