code review 6501077: runtime: refactor goroutine blocking

runtime: refactor goroutine blocking
The change is a preparation for the new scheduler.
It introduces runtime.park() function,
that will atomically unlock the mutex and park the goroutine.
It will allow to remove the racy readyonstop flag
that is difficult to implement w/o the global scheduler mutex.

[dvyukov, 2012-09-03 13:48:41]

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

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

[remyoudompheng, 2012-09-03 19:38:08]

http://codereview.appspot.com/6501077/diff/4/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/6501077/diff/4/src/pkg/runtime/proc.c#newcode954
src/pkg/runtime/proc.c:954: void
it could be a good idea to document the purpose of this function:

runtime·park puts the current goroutine into a waiting state and unlocks...

especially if it must become more complex. Could there be an interest in
defining a

typedef void (*unlockfunc)(void*); 

so that the signatures and casts are more readable?

[dvyukov, 2012-09-04 07:40:14]

On 2012/09/03 19:38:08, remyoudompheng wrote:
> http://codereview.appspot.com/6501077/diff/4/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
> http://codereview.appspot.com/6501077/diff/4/src/pkg/runtime/proc.c#newcode954
> src/pkg/runtime/proc.c:954: void
> it could be a good idea to document the purpose of this function:
> 
> runtime·park puts the current goroutine into a waiting state and unlocks...

Done.

> especially if it must become more complex. Could there be an interest in
> defining a
> 
> typedef void (*unlockfunc)(void*); 
> 
> so that the signatures and casts are more readable?

Yeah, the casts are ugly. I've just defined the function as void(*)(Lock*), so
now there is only one cast in select. PTAL.

[dave_cheney.net, 2012-09-04 07:57:19]

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:33: runtime·unlock(&timers);
Why did you move the guard outside the call to addtimer? addtimer might cause a
stack split which would hold the lock longer than necessary.

[dvyukov, 2012-09-04 08:10:06]

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:33: runtime·unlock(&timers);
On 2012/09/04 07:57:19, dfc wrote:
> Why did you move the guard outside the call to addtimer? addtimer might cause
a
> stack split which would hold the lock longer than necessary.

Because runtime·tsleep() needs to call park() instead of unlock().

[dave_cheney.net, 2012-09-04 10:36:50]

This looks pretty good. Thank you.

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:33: runtime·unlock(&timers);
On 2012/09/04 08:10:06, dvyukov wrote:
> On 2012/09/04 07:57:19, dfc wrote:
> > Why did you move the guard outside the call to addtimer? addtimer might
cause
> a
> > stack split which would hold the lock longer than necessary.
> 
> Because runtime·tsleep() needs to call park() instead of unlock().
> 

Thanks, I understand now.

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:58: // The caller must have set g->status and
g->waitreason.
Is this line of the comment correct anymore ? runtime.park is in charge of
g->status and g->waitreason.

[dvyukov, 2012-09-04 10:54:09]

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc
File src/pkg/runtime/time.goc (right):

http://codereview.appspot.com/6501077/diff/7003/src/pkg/runtime/time.goc#newc...
src/pkg/runtime/time.goc:58: // The caller must have set g->status and
g->waitreason.
On 2012/09/04 10:36:50, dfc wrote:
> Is this line of the comment correct anymore ? runtime.park is in charge of
> g->status and g->waitreason.

Done.

[rsc, 2012-09-11 02:11:31]

LGTM

[dvyukov, 2012-09-18 17:15:54]

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

runtime: refactor goroutine blocking
The change is a preparation for the new scheduler.
It introduces runtime.park() function,
that will atomically unlock the mutex and park the goroutine.
It will allow to remove the racy readyonstop flag
that is difficult to implement w/o the global scheduler mutex.

R=rsc, remyoudompheng, dave
CC=golang-dev
http://codereview.appspot.com/6501077