code review 110680044: runtime: call rfork on scheduler stack on Plan 9

runtime: call rfork on scheduler stack on Plan 9

A race exists between the parent and child processes after a fork.
The child needs to access the new M pointer passed as an argument
but the parent may have already returned and clobbered it.

Previously, we avoided this by saving the necessary data into
registers before the rfork system call but this isn't guaranteed
to work because Plan 9 makes no promises about the register state
after a system call. Only the 386 kernel seems to save them.
For amd64 and arm, this method won't work.

We eliminate the race by allocating stack space for the scheduler
goroutines (g0) in the per-process copy-on-write stack segment and
by only calling rfork on the scheduler stack.

[aram, 2014-07-20 10:02:24]

Thanks for working on this.

I am quite confused by the use of runtime·asmcgocall. Why go through all that
trouble when we can just switch the stack manually in a few machine
instructions?

Thanks again.

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:715: MOVQ	BX, 0(SP)	// first argument for Plan 9
amd64
Presumably we need something like this for asm_386.s as well.

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/os_plan9.c
File src/pkg/runtime/os_plan9.c (right):

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/os_plan9....
src/pkg/runtime/os_plan9.c:290: runtime·asmcgocall(newosproc0, mp);
It is very unexpected to call a C function from runtime·asmcgocall.

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/sys_plan9...
File src/pkg/runtime/sys_plan9_386.s (right):

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/sys_plan9...
src/pkg/runtime/sys_plan9_386.s:97: TEXT runtime·procstart(SB),NOSPLIT,$0
Please call this function runtime·tstart_plan9, for consistency with the other
platforms.

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/sys_plan9...
src/pkg/runtime/sys_plan9_386.s:104: SUBL	$(64*1024), AX		// stack size
I think we can put 16MB here.

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/sys_plan9...
File src/pkg/runtime/sys_plan9_amd64.s (right):

https://codereview.appspot.com/110680044/diff/80001/src/pkg/runtime/sys_plan9...
src/pkg/runtime/sys_plan9_amd64.s:112: SUBQ	$(64*1024), AX		// stack size
I think we can put 16MB here.

[0intro, 2014-07-20 12:53:58]

Thanks.

The tests are passing successfully on 386 Plan 9.

Increasing the stack to 16MB would be more coherent
with the size of the user stack on Plan 9.

[ality, 2014-07-20 14:52:45]

PTAL

aram@mgk.ro once said:
> Thanks for working on this.
> 
> I am quite confused by the use of runtime·asmcgocall. Why go through all
> that trouble when we can just switch the stack manually in a few machine
> instructions?

Sure, we can switch the stack with a few instructions but there's a bit
more than I care for if we do the fork and all the checks in assembly.
But you're right about using runtime·asmcgocall. I've switched to using
runtime·mcall which I think makes the intent clear.

> src/pkg/runtime/sys_plan9_386.s:97: TEXT
> runtime·procstart(SB),NOSPLIT,$0
> Please call this function runtime·tstart_plan9, for consistency with the
> other platforms.

Done.

> src/pkg/runtime/sys_plan9_386.s:104: SUBL	$(64*1024), AX		// stack size
> I think we can put 16MB here.

16MB is no good. That would put us past the end of the stack
segment since SP is already a little forward of the TOS at
the time of rfork. The first m gets a 64K stack (in _rt0_go)
so I just copied that. I can probably bump it up to a few
megs if you prefer.

Cheers,
  Anthony

[aram, 2014-07-20 15:05:51]

LGTM

Let's allow for more time to test this on amd64 before comitting,
though.

> 16MB is no good. That would put us past the end of the stack segment
> since SP is already a little forward of the TOS at the time of
> rfork. The first m gets a 64K stack (in _rt0_go) so I just copied
> that. I can probably bump it up to a few megs if you prefer.

You are right, we could use USTKTOP-USTKSIZ, but I changed my mind
again. We don't need much stack, so if we use too much, it's a bug.
Using the same value as in _rt0_go would allow us to catch these
kind of bugs.

64k is fine, thanks.

https://codereview.appspot.com/110680044/diff/100001/src/pkg/runtime/os_plan9.c
File src/pkg/runtime/os_plan9.c (right):

https://codereview.appspot.com/110680044/diff/100001/src/pkg/runtime/os_plan9...
src/pkg/runtime/os_plan9.c:301: runtime·mcall(mnewosproc);
I apologise, but I think runtime·asmcgocall is better after all. This is even
weirder and more complicated.

[0intro, 2014-07-20 16:02:01]

I've tried your latest change and so far it seems to
work fine on 386 Plan 9.

By the way, it seems the "Delta from patch set" feature
is currently broken in Rietveld.

[mischief, 2014-07-21 05:35:48]

On 2014/07/20 16:02:01, 0intro wrote:
> I've tried your latest change and so far it seems to
> work fine on 386 Plan 9.
> 

seems OK on the amd64 builder. the timeouts remain.

[aram, 2014-07-21 09:19:18]

Let's mail it, wait a few days, and ship it.

[0intro, 2014-07-21 09:32:15]

> Let's mail it, wait a few days, and ship it.

Agreed!

[aram, 2014-07-23 09:21:23]

Ping.

[0intro, 2014-07-25 14:27:24]

Ping.

[0intro, 2014-07-31 08:21:58]

Ping.

[ality, 2014-08-28 08:35:56]

Hello aram@mgk.ro, 0intro@gmail.com, mischief@offblast.org (cc:
golang-codereviews@googlegroups.com),

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

[aram, 2014-08-28 08:55:39]

LGTM

[0intro, 2014-08-28 09:20:52]

LGTM

We should wait for Russ, since he is currently working on the assembly files.

-- 
David du Colombier

[0intro, 2014-08-28 09:33:22]

And we should also submit CL 132320043 before, of course.

-- 
David du Colombier

[ality, 2014-08-28 23:08:09]

[+rsc]

[rsc, 2014-09-07 13:05:04]

LGTM
especially if the onM goes away but even if not

https://codereview.appspot.com/110680044/diff/160001/src/pkg/runtime/os_plan9.c
File src/pkg/runtime/os_plan9.c (right):

https://codereview.appspot.com/110680044/diff/160001/src/pkg/runtime/os_plan9...
src/pkg/runtime/os_plan9.c:294: runtime·onM(newosproc_m);
I don't understand this. Isn't newosproc only called from the m stack already?

[ality, 2014-09-07 23:29:25]

https://codereview.appspot.com/110680044/diff/160001/src/pkg/runtime/os_plan9.c
File src/pkg/runtime/os_plan9.c (right):

https://codereview.appspot.com/110680044/diff/160001/src/pkg/runtime/os_plan9...
src/pkg/runtime/os_plan9.c:294: runtime·onM(newosproc_m);
On 2014/09/07 13:05:04, rsc wrote:
> I don't understand this. Isn't newosproc only called from the m stack already?

Is that supposed to be the invariant? For the sysmon thread, runtime.main calls
newm which then calls runtime.newosproc. All of that is on the user g stack that
was set up in runtime.rt0_go.

I think there are a couple other places where newm can be called with a user g.
Mostly because of runtime.starttheworld.

[rsc, 2014-09-08 03:05:39]

okay. clearly i am misremembering. :-)

[ality, 2014-09-09 21:00:41]

Russ Cox <rsc@golang.org> once said:
> okay. clearly i am misremembering. :-)

Now that CL 133700043 is in, there shouldn't be any
use of newosproc on a Go stack so I've removed the
call to onM.

I also adjusted this CL to use the new stack boundary
names from CL 137410043.

There are no other changes and I will submit shortly.

  Anthony

[0intro, 2014-09-09 21:14:23]

Thanks. I've just tried your latest changes on 386 Plan 9.

LGTM

[ality, 2014-09-10 00:19:15]

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

runtime: call rfork on scheduler stack on Plan 9

A race exists between the parent and child processes after a fork.
The child needs to access the new M pointer passed as an argument
but the parent may have already returned and clobbered it.

Previously, we avoided this by saving the necessary data into
registers before the rfork system call but this isn't guaranteed
to work because Plan 9 makes no promises about the register state
after a system call. Only the 386 kernel seems to save them.
For amd64 and arm, this method won't work.

We eliminate the race by allocating stack space for the scheduler
goroutines (g0) in the per-process copy-on-write stack segment and
by only calling rfork on the scheduler stack.

LGTM=aram, 0intro, rsc
R=aram, 0intro, mischief, rsc
CC=golang-codereviews
https://codereview.appspot.com/110680044

[gobot, 2014-09-10 03:07:08]

This CL appears to have broken the windows-amd64-perf builder.
See http://build.golang.org/log/8e72aa5354c8b07748d3b05d393c750a2c8cd430