code review 10264044: runtime: preempt goroutines for GC

runtime: preempt goroutines for GC
The last patch for preemptive scheduler,
with this change stoptheworld issues preemption
requests every 100us.
Update issue 543.

[dvyukov, 2013-06-13 15:21:00]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-06-13 15:21:05]

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

Please take another look.

[dvyukov, 2013-06-17 11:32:19]

On 2013/06/13 15:21:05, dvyukov wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

ping

[DMorsing, 2013-06-17 11:54:24]

Is this intended for go 1.2? I haven't seen its inclusion mentioned anywhere.

I'll take a closer look later, but initial thoughts are that it would be a good
idea to add a compile-time debug flag. Preemption is a big change to our current
execution model and a flag will help once we start debugging after the freeze.

[dvyukov, 2013-06-17 12:01:31]

On Mon, Jun 17, 2013 at 3:54 PM,  <daniel.morsing@gmail.com> wrote:
> Is this intended for go 1.2? I haven't seen its inclusion mentioned
> anywhere.

Yes, it's for Go1.2 and I believe it was mentioned in few places.
There was a discussion about design on golang-dev. And it should be in
Russ' Go1.2. list of big things.


> I'll take a closer look later, but initial thoughts are that it would be
> a good idea to add a compile-time debug flag. Preemption is a big change
> to our current execution model and a flag will help once we start
> debugging after the freeze.


It's easy to turn off in source code. But if we will need to tell
people to turn it off and retest their crashing programs, then a const
in runtime.h will be definitely better. Will do.

[rsc, 2013-06-17 12:05:55]

Does this depend on any other pending CLs, or are all its prerequisites
submitted already?

Thanks.
Russ

[dvyukov, 2013-06-17 12:07:47]

On Mon, Jun 17, 2013 at 4:05 PM, Russ Cox <rsc@golang.org> wrote:
> Does this depend on any other pending CLs, or are all its prerequisites
> submitted already?


All prerequisites are submitted. It includes a test that requires
preemption and it passes with this change.

[rsc, 2013-06-17 12:31:03]

I'm very happy to see this coming to completion, and I will review this CL.
However, Rob and I keep running into mysterious runtime behaviors that need
to be fixed before we can make more changes. I am afraid that the runtime
is starting to spiral out of control, and we need to steady it a bit before
making more changes. See golang.org/issue/5723 for details.

Russ

[dvyukov, 2013-06-27 18:07:58]

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

Please take another look.

[dvyukov, 2013-06-27 18:08:37]

synced to tip
ready for review

[dvyukov, 2013-06-27 18:12:10]

There is one functionality that was there, but I don't know yet how to implement
it now:
If newstack finds gp->stackguard0 == StackPreempt, but it can not be preempted
right now (m->locks != 0), then newstack lefts gp->stackguard0 == StackPreempt.
Then the goroutine will try to preempt itself on the next split stack check.
Currently the goroutine retries from function beginning after morestack, so I
can not leave StackPreempt (will cause infinite loop).

[rsc, 2013-06-27 19:17:06]

On 2013/06/27 18:12:10, dvyukov wrote:
> There is one functionality that was there, but I don't know yet how to
implement
> it now:
> If newstack finds gp->stackguard0 == StackPreempt, but it can not be preempted
> right now (m->locks != 0), then newstack lefts gp->stackguard0 ==
StackPreempt.
> Then the goroutine will try to preempt itself on the next split stack check.
> Currently the goroutine retries from function beginning after morestack, so I
> can not leave StackPreempt (will cause infinite loop).

You could put it back when m->locks reaches 0.

Russ

[rsc, 2013-06-27 19:37:53]

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

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:386: // wait for remaining P's to stop voluntary
voluntarily

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc_test.go
File src/pkg/runtime/proc_test.go (right):

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc_test....
src/pkg/runtime/proc_test.go:161: var preempt = func() int {
Why is this not

func preempt() int {
?

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:243: if(oldstatus == Grunning && m->locks == 0 &&
m->mallocing == 0 && m->gcing == 0 &&
Surely you have missed a condition. 
How can this be made simpler?

How can oldstatus not be Grunning?
How can m->p be nil?
How can m->p->status not be Prunning?

[dvyukov, 2013-06-27 19:47:51]

On Thu, Jun 27, 2013 at 11:17 PM,  <rsc@golang.org> wrote:
> On 2013/06/27 18:12:10, dvyukov wrote:
>>
>> There is one functionality that was there, but I don't know yet how to
>
> implement
>>
>> it now:
>> If newstack finds gp->stackguard0 == StackPreempt, but it can not be
>
> preempted
>>
>> right now (m->locks != 0), then newstack lefts gp->stackguard0 ==
>
> StackPreempt.
>>
>> Then the goroutine will try to preempt itself on the next split stack
>
> check.
>>
>> Currently the goroutine retries from function beginning after
>
> morestack, so I
>>
>> can not leave StackPreempt (will cause infinite loop).
>
>
> You could put it back when m->locks reaches 0.


That's what I've tried to avoid -- additional checks on fast paths
(malloc, lock/unlock, etc)...
I will think a bit more, and then probably will do as you suggest.
This issue it not blocking this CL.

[dvyukov, 2013-06-27 19:54:45]

On Thu, Jun 27, 2013 at 11:37 PM,  <rsc@golang.org> wrote:
>
> https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
>
>
https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc.c#new...
> src/pkg/runtime/proc.c:386: // wait for remaining P's to stop voluntary
> voluntarily
>
>
https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc_test.go
> File src/pkg/runtime/proc_test.go (right):
>
>
https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc_test....
> src/pkg/runtime/proc_test.go:161: var preempt = func() int {
> Why is this not
>
> func preempt() int {
> ?


It must not be inlined and must have split check.
"var preempt = func()" is safer with respect to possible future
optimizations in the compiler.



> https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c
> File src/pkg/runtime/stack.c (right):
>
>
https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c#ne...
> src/pkg/runtime/stack.c:243: if(oldstatus == Grunning && m->locks == 0
> && m->mallocing == 0 && m->gcing == 0 &&
> Surely you have missed a condition.

:)

> How can this be made simpler?
>
> How can oldstatus not be Grunning?

It can be Gsyscall if called from enter/exitsyscall.

> How can m->p be nil?

It also can happen at least in enter/exitsyscall.

> How can m->p->status not be Prunning?

same
entersyscall leaves m->p set while it does not own the P

there may be other than enter/exitsyscall cases, this is just he most obvious.

If we would call enter/exitsyscall only on g0, it could make this
condition simpler.

but, yes, this check looks quite defensive...

[rsc, 2013-06-27 20:00:41]

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:200: 
How about, right here,

// If we are preempted during work on behalf of a system call, ignore.
if(gp->stackguard0 == StackPreempt && gp->status != Grunning) {
    gp->stackguard0 = gp->stackguard;
    runtime.gogo(&gp->sched);
}

Doing that very early means that we should be able to drop oldstatus, and the
possibility that m->p == nil, and the possibility that m->p->status != Prunning,
right?

I sent you a CL so that this is safe even in the reflectcall case.

[rsc, 2013-06-27 20:20:17]

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:200: 
To be clear, what I meant by "safe" is that it's always legal to
runtime.gogo(&gp->sched) to back out of a newstack. You would still need to
figure out what conditions need to be backed out of quickly. But I think doing
the check here at the very top, before we touch anything else like gp->status,
will keep the rest of the code clean.

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:238: if(gp->stackguard0 == StackPreempt) {
The actual preemption can still happen here, in the case where we have an
ordinary running goroutine. No magic necessary.

if(gp->stackguard0 == StackPreempt) {
    gp->stackguard = gp->stackguard0;
    if(gp == m->g0)
        runtime.throw("runtime: preempt g0");
    gp->status = oldstatus;
    if(m->locks == 0 || m->mallocing == 0 || m->gcing == 0) {
        // Remember but delay the preemption.
        // Let the goroutine keep running for now.
        m->preempt = 1;
        runtime.gogo(&gp->sched);
    }
    // Act like goroutine called runtime.Gosched.
    // Does not return.
    runtime.gosched0(sp);
}

[dvyukov, 2013-06-27 21:07:41]

PTAL

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

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:386: // wait for remaining P's to stop voluntary
On 2013/06/27 19:37:53, rsc wrote:
> voluntarily

Done.

[rsc, 2013-06-27 21:08:41]

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.h#ne...
src/pkg/runtime/stack.h:110: StackPreempt = (uintptr)(intptr)(int32)0xfffffade,
This cannot be an enum. Also, (uintptr)(intptr)(int32) is no different than
(uintptr)(int32).

#define StackPreempt ((uintptr)-3)

[rsc, 2013-06-27 21:10:45]

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/proc_test.go
File src/pkg/runtime/proc_test.go (right):

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/proc_test....
src/pkg/runtime/proc_test.go:160: // The function is used to test preemption at
split stack checks.
// Declaring a var avoids inlining at the call site.

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:254: gp->stackguard0 = gp->stackguard;
Don't you want to do this in both cases?
I'd move it up to the first line of the if body.

[dvyukov, 2013-06-27 21:18:15]

PTAL

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/10264044/diff/30001/src/pkg/runtime/stack.h#ne...
src/pkg/runtime/stack.h:110: StackPreempt = (uintptr)(intptr)(int32)0xfffffade,
On 2013/06/27 21:08:41, rsc wrote:
> This cannot be an enum. Also, (uintptr)(intptr)(int32) is no different than
> (uintptr)(int32).
> 
> #define StackPreempt ((uintptr)-3)
> 

Done.

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/proc_test.go
File src/pkg/runtime/proc_test.go (right):

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/proc_test....
src/pkg/runtime/proc_test.go:160: // The function is used to test preemption at
split stack checks.
On 2013/06/27 21:10:46, rsc wrote:
> // Declaring a var avoids inlining at the call site.

Done.

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/10264044/diff/45002/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:254: gp->stackguard0 = gp->stackguard;
On 2013/06/27 21:10:46, rsc wrote:
> Don't you want to do this in both cases?
> I'd move it up to the first line of the if body.

No, I don't. It is not necessary, gosched() must be able to handle this.

[rsc, 2013-06-27 21:21:36]

LGTM

Please wait until tomorrow to submit. I'd like the builders to catch up and
get past my gp->sched.sp change and be "ok" first.

[dvyukov, 2013-06-28 13:52:36]

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

runtime: preempt goroutines for GC
The last patch for preemptive scheduler,
with this change stoptheworld issues preemption
requests every 100us.
Update issue 543.

R=golang-dev, daniel.morsing, rsc
CC=golang-dev
https://codereview.appspot.com/10264044