code review 10792043: runtime: more reliable preemption

runtime: more reliable preemption
Currently preemption signal g->stackguard0==StackPreempt
can be lost if it is received when preemption is disabled
(e.g. m->lock!=0). This change duplicates the preemption
signal in g->preempt and restores g->stackguard0
when preemption is enabled.
Update issue 543.

[dvyukov, 2013-06-29 17:58:59]

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-07-01 21:39:55]

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex.c
File src/pkg/runtime/lock_futex.c (right):

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex....
src/pkg/runtime/lock_futex.c:103: if(g->preempt)  // restore the preemption
request in case we've cleared it in newstack
if(m->locks == 0 && g->preempt)

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_sema.c
File src/pkg/runtime/lock_sema.c (right):

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_sema.c...
src/pkg/runtime/lock_sema.c:116: if(g->preempt)  // restore the preemption
request in case we've cleared it in newstack
if(m->locks == 0 && g->preempt)

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

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:297: if(g->preempt)  // restore the preemption request in
case we've cleared it in newstack
if(m->locks == 0 && g->preempt)

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:479: m->locks--;
if(m->locks == 0 && g->preempt)

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:1621: if(g->preempt)  // restore the preemption request
in case we've cleared it in newstack
if(m->locks == 0 && g->preempt)

[dvyukov, 2013-07-16 17:11:46]

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex.c
File src/pkg/runtime/lock_futex.c (right):

https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex....
src/pkg/runtime/lock_futex.c:103: if(g->preempt)  // restore the preemption
request in case we've cleared it in newstack
On 2013/07/01 21:39:56, rsc wrote:
> if(m->locks == 0 && g->preempt)

If we want to restore g->stackguard0 = StackPreempt exactly when preemption is
possible then the full condition should be along the lines of:
if(g->preempt && g->status == Grunning && m->locks == 0 && m->mallocing == 0 &&
m->gcing == 0)
I do not think it's worth it. We do not have deep lock/mallocing nesting, so the
current check should be a good approximation.

[rsc, 2013-07-16 17:39:13]

On 2013/07/16 17:11:46, dvyukov wrote:
> https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex.c
> File src/pkg/runtime/lock_futex.c (right):
> 
>
https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex....
> src/pkg/runtime/lock_futex.c:103: if(g->preempt)  // restore the preemption
> request in case we've cleared it in newstack
> On 2013/07/01 21:39:56, rsc wrote:
> > if(m->locks == 0 && g->preempt)
> 
> If we want to restore g->stackguard0 = StackPreempt exactly when preemption is
> possible then the full condition should be along the lines of:
> if(g->preempt && g->status == Grunning && m->locks == 0 && m->mallocing == 0
&&
> m->gcing == 0)
> I do not think it's worth it. We do not have deep lock/mallocing nesting, so
the
> current check should be a good approximation.

Typically the preempt is set when only one of those conditions failed. You might
as well check the condition you just changed (namely m->locks).

[dvyukov, 2013-07-17 09:34:17]

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

Please take another look.

[dvyukov, 2013-07-17 09:34:33]

On 2013/07/16 17:39:13, rsc wrote:
> On 2013/07/16 17:11:46, dvyukov wrote:
> >
https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex.c
> > File src/pkg/runtime/lock_futex.c (right):
> > 
> >
>
https://codereview.appspot.com/10792043/diff/6001/src/pkg/runtime/lock_futex....
> > src/pkg/runtime/lock_futex.c:103: if(g->preempt)  // restore the preemption
> > request in case we've cleared it in newstack
> > On 2013/07/01 21:39:56, rsc wrote:
> > > if(m->locks == 0 && g->preempt)
> > 
> > If we want to restore g->stackguard0 = StackPreempt exactly when preemption
is
> > possible then the full condition should be along the lines of:
> > if(g->preempt && g->status == Grunning && m->locks == 0 && m->mallocing == 0
> &&
> > m->gcing == 0)
> > I do not think it's worth it. We do not have deep lock/mallocing nesting, so
> the
> > current check should be a good approximation.
> 
> Typically the preempt is set when only one of those conditions failed. You
might
> as well check the condition you just changed (namely m->locks).

done
ptal

[rsc, 2013-07-17 16:48:39]

LGTM

[rsc, 2013-07-17 16:52:40]

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

runtime: more reliable preemption
Currently preemption signal g->stackguard0==StackPreempt
can be lost if it is received when preemption is disabled
(e.g. m->lock!=0). This change duplicates the preemption
signal in g->preempt and restores g->stackguard0
when preemption is enabled.
Update issue 543.

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

Committer: Russ Cox <rsc@golang.org>