code review 11575044: runtime: do not split stacks in syscall status

runtime: do not split stacks in syscall status
Split stack checks (morestack) corrupt g->sched,
but g->sched must be preserved consistent for GC/traceback.
The change implements runtime.notetsleepg function,
which does entersyscall/exitsyscall and is carefully arranged
to not call any split functions in between.

[rsc, 2013-07-22 20:34:44]

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c#n...
src/pkg/runtime/cgocall.c:241: #pragma textflag,7
s/,/ /
I am surprised the comma even works, but it's not what we usually write.

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c#n...
src/pkg/runtime/cgocall.c:247: if(m->racecall) {
I think this is wrong. If the code you are calling panics, the defer needs to be
run so that unwindm will run. This is not a new bug. I think this should be:

if(m->racecall)
    runtime.cgocallbackg1();
else {
    runtime.exitsyscall();
    runtime.cgocallbackg1();
    runtime.entersyscall();
}

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c#n...
src/pkg/runtime/cgocall.c:249: cb =
(CallbackArgs*)((byte*)m->g0->sched.sp+4*sizeof(void*));
should be 3, right?

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

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/lock_futex....
src/pkg/runtime/lock_futex.c:171: int64 deadline, now;
This body needs some good comments about why the bodies above are inlined here.
Explain the changes too: why are there no calls to setprof?

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

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/lock_sema.c...
src/pkg/runtime/lock_sema.c:244: M *mp;
Same, and this one is even worse.
Do we really need to make another copy of this code?

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_darwin.c
File src/pkg/runtime/os_darwin.c (right):

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_darwin.c...
src/pkg/runtime/os_darwin.c:25: #pragma textflag,7
s/,/ /
I'm going to stop marking these, but please grep for them.

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_freebsd.c
File src/pkg/runtime/os_freebsd.c (right):

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_freebsd....
src/pkg/runtime/os_freebsd.c:63: for(secs = 1LL<<30; secs != 0; secs >>= 2) {
Are you sure it's >>= 2 and not >>= 1?
You should have enough stack to pull this into a function and call that one copy
of the code from all the different os files.

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

https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:1450: // Try to re-acquire the last P.
Can you refactor this slightly so that it can be

if(exitsyscallp()) {
    m->p->tick++;
    g->status = Grunning;
    ...
    return;
}

and then exitsyscallp would do both the "Try to re-acquire the last P" and also
"Try to get any other idle P".
It would return true on success, false on failure.
Then there are not two copies of the success code.

[dvyukov, 2013-07-24 15:19:42]

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/

[dvyukov, 2013-07-24 15:21:56]

On 2013/07/22 20:34:44, rsc wrote:
> https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c
> File src/pkg/runtime/cgocall.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c#n...
> src/pkg/runtime/cgocall.c:241: #pragma textflag,7
> s/,/ /
> I am surprised the comma even works, but it's not what we usually write.
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c#n...
> src/pkg/runtime/cgocall.c:247: if(m->racecall) {
> I think this is wrong. If the code you are calling panics, the defer needs to
be
> run so that unwindm will run. This is not a new bug. I think this should be:
> 
> if(m->racecall)
>     runtime.cgocallbackg1();
> else {
>     runtime.exitsyscall();
>     runtime.cgocallbackg1();
>     runtime.entersyscall();
> }
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/cgocall.c#n...
> src/pkg/runtime/cgocall.c:249: cb =
> (CallbackArgs*)((byte*)m->g0->sched.sp+4*sizeof(void*));
> should be 3, right?
> 
> https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/lock_futex.c
> File src/pkg/runtime/lock_futex.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/lock_futex....
> src/pkg/runtime/lock_futex.c:171: int64 deadline, now;
> This body needs some good comments about why the bodies above are inlined
here.
> Explain the changes too: why are there no calls to setprof?
> 
> https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/lock_sema.c
> File src/pkg/runtime/lock_sema.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/lock_sema.c...
> src/pkg/runtime/lock_sema.c:244: M *mp;
> Same, and this one is even worse.
> Do we really need to make another copy of this code?
> 
> https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_darwin.c
> File src/pkg/runtime/os_darwin.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_darwin.c...
> src/pkg/runtime/os_darwin.c:25: #pragma textflag,7
> s/,/ /
> I'm going to stop marking these, but please grep for them.
> 
> https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_freebsd.c
> File src/pkg/runtime/os_freebsd.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/os_freebsd....
> src/pkg/runtime/os_freebsd.c:63: for(secs = 1LL<<30; secs != 0; secs >>= 2) {
> Are you sure it's >>= 2 and not >>= 1?
> You should have enough stack to pull this into a function and call that one
copy
> of the code from all the different os files.
> 
> https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/2039/src/pkg/runtime/proc.c#newc...
> src/pkg/runtime/proc.c:1450: // Try to re-acquire the last P.
> Can you refactor this slightly so that it can be
> 
> if(exitsyscallp()) {
>     m->p->tick++;
>     g->status = Grunning;
>     ...
>     return;
> }
> 
> and then exitsyscallp would do both the "Try to re-acquire the last P" and
also
> "Try to get any other idle P".
> It would return true on success, false on failure.
> Then there are not two copies of the success code.


Done
PTAL

[dvyukov, 2013-07-26 10:17:13]

ping

[dvyukov, 2013-07-26 10:18:06]

https://codereview.appspot.com/11575044/diff/59001/src/pkg/runtime/os_windows.c
File src/pkg/runtime/os_windows.c (right):

https://codereview.appspot.com/11575044/diff/59001/src/pkg/runtime/os_windows...
src/pkg/runtime/os_windows.c:175: ns = runtime·timediv(ns, 1000000, nil);
note to myself: add if(ns == 0) ns = 1;

[dvyukov, 2013-07-26 13:12:39]

On 2013/07/26 10:18:06, dvyukov wrote:
>
https://codereview.appspot.com/11575044/diff/59001/src/pkg/runtime/os_windows.c
> File src/pkg/runtime/os_windows.c (right):
> 
>
https://codereview.appspot.com/11575044/diff/59001/src/pkg/runtime/os_windows...
> src/pkg/runtime/os_windows.c:175: ns = runtime·timediv(ns, 1000000, nil);
> note to myself: add if(ns == 0) ns = 1;

done in patchset 24

more fixes after testing on windows in patchset 25

[rsc, 2013-07-29 17:26:46]

LGTM

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/futex_test.go
File src/pkg/runtime/futex_test.go (right):

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/futex_test...
src/pkg/runtime/futex_test.go:5: // Race detector emits calls to split stack
functions between entersyscall/exitsyscall.
// Futex is only available on Linux and FreeBSD.
// The race detector emits calls to split stack functions so it breaks the test.
// +build linux freebsd
// +build !race

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

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/lock_sema....
src/pkg/runtime/lock_sema.c:178: // deadline and mp must be plain local vars in
this function,
Get rid of the stars.
Just declare int64 deadline, M *mp in the argument list.

// Conceptually, deadline and mp are local variables.
// They are passed as arguments so that the space for them
// does not count against our nosplit stack sequence.

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

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:253: if(oldstatus != Grunning || m->locks ||
m->mallocing || m->gcing || m->p->status != Prunning || m->racecall) {
What sets m->racecall? Can it increment m->locks instead? Every new condition
here makes this harder to think about.

[dvyukov, 2013-07-29 18:21:54]

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/futex_test.go
File src/pkg/runtime/futex_test.go (right):

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/futex_test...
src/pkg/runtime/futex_test.go:5: // Race detector emits calls to split stack
functions between entersyscall/exitsyscall.
On 2013/07/29 17:26:46, rsc wrote:
> // Futex is only available on Linux and FreeBSD.
> // The race detector emits calls to split stack functions so it breaks the
test.
> // +build linux freebsd
> // +build !race

Done.

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

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/lock_sema....
src/pkg/runtime/lock_sema.c:178: // deadline and mp must be plain local vars in
this function,
On 2013/07/29 17:26:46, rsc wrote:
> Get rid of the stars.
> Just declare int64 deadline, M *mp in the argument list.
> 
> // Conceptually, deadline and mp are local variables.
> // They are passed as arguments so that the space for them
> // does not count against our nosplit stack sequence.

Aha!
This is a new not-yet-formed idiom :)
Done

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

https://codereview.appspot.com/11575044/diff/80001/src/pkg/runtime/stack.c#ne...
src/pkg/runtime/stack.c:253: if(oldstatus != Grunning || m->locks ||
m->mallocing || m->gcing || m->p->status != Prunning || m->racecall) {
On 2013/07/29 17:26:46, rsc wrote:
> What sets m->racecall? Can it increment m->locks instead? Every new condition
> here makes this harder to think about.


I've found a better place for this and added a comment explaining why we need
it:

runtime·cgocallbackg(void)
{
	if(g != m->curg) {
		runtime·prints("runtime: bad g in cgocallback");
		runtime·exit(2);
	}

	if(m->racecall) {
		// We were not in syscall, so no need to call runtime·exitsyscall.
		// However we must set m->locks for the following reason.
		// Race detector runtime makes __tsan_symbolize cgo callback
		// holding internal mutexes. The mutexes are not cooperative with Go
scheduler.
		// So if we deschedule a goroutine that holds race detector internal mutex
		// (e.g. preempt it), another goroutine will deadlock trying to acquire the
same mutex.
		m->locks++;
		runtime·cgocallbackg1();
		m->locks--;
	} else {

[dvyukov, 2013-07-29 18:22:18]

crossing fingers and submitting

[dvyukov, 2013-07-29 18:22:44]

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

runtime: do not split stacks in syscall status
Split stack checks (morestack) corrupt g->sched,
but g->sched must be preserved consistent for GC/traceback.
The change implements runtime.notetsleepg function,
which does entersyscall/exitsyscall and is carefully arranged
to not call any split functions in between.

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

[dvyukov, 2013-07-29 18:48:55]

On Mon, Jul 29, 2013 at 10:22 PM,  <dvyukov@google.com> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=cb7c919fe554 ***
>
>
> runtime: do not split stacks in syscall status
> Split stack checks (morestack) corrupt g->sched,
> but g->sched must be preserved consistent for GC/traceback.
> The change implements runtime.notetsleepg function,
> which does entersyscall/exitsyscall and is carefully arranged
> to not call any split functions in between.
>
> R=rsc
> CC=golang-dev
> https://codereview.appspot.com/11575044
>
>
> https://codereview.appspot.com/11575044/


Not sure whether this netbsd/amd64 failure is related or not:
http://build.golang.org/log/d365fa6d033bc272a5662a5630b5ff03d8fb3d90

netbsd/386 failure seems to be unrelated:
http://build.golang.org/log/51d557fdcbff67f0cb8e318be1a2c8c6e081c9f5

[dvyukov, 2013-07-29 19:29:57]

This is a real one:

fatal error: runtime: stack split during syscall

goroutine 2 [stack split]:
_addv(0xb6fa0f28, 0xd0a5112e, 0x13156d6e, 0xf8475800, 0xd)
/usr/local/go/src/pkg/runtime/vlrt_arm.c:66 fp=0xb6fa0ef8
notetsleep(0xb6fa0f9c, 0xf8475800, 0xd, 0x0, 0x0, ...)
/usr/local/go/src/pkg/runtime/lock_futex.c:156 +0xd0 fp=0xb6fa0f18
runtime.notetsleepg(0xb6fa0f9c, 0xf8475800, 0xd)
/usr/local/go/src/pkg/runtime/lock_futex.c:197 +0x74 fp=0xb6fa0f3c
runtime.MHeap_Scavenger()
/usr/local/go/src/pkg/runtime/mheap.c:463 +0xec fp=0xb6fa0fcc
runtime.goexit()
/usr/local/go/src/pkg/runtime/proc.c:1332 fp=0xb6fa0fcc
created by runtime.main
/usr/local/go/src/pkg/runtime/proc.c:168

http://build.golang.org/log/b01f70864b1feaeef16300bc14349f3ade7cbe6f

And I don't know what to do with it yet.



On Mon, Jul 29, 2013 at 10:48 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Jul 29, 2013 at 10:22 PM,  <dvyukov@google.com> wrote:
>> *** Submitted as
>> https://code.google.com/p/go/source/detail?r=cb7c919fe554 ***
>>
>>
>> runtime: do not split stacks in syscall status
>> Split stack checks (morestack) corrupt g->sched,
>> but g->sched must be preserved consistent for GC/traceback.
>> The change implements runtime.notetsleepg function,
>> which does entersyscall/exitsyscall and is carefully arranged
>> to not call any split functions in between.
>>
>> R=rsc
>> CC=golang-dev
>> https://codereview.appspot.com/11575044
>>
>>
>> https://codereview.appspot.com/11575044/
>
>
> Not sure whether this netbsd/amd64 failure is related or not:
> http://build.golang.org/log/d365fa6d033bc272a5662a5630b5ff03d8fb3d90
>
> netbsd/386 failure seems to be unrelated:
> http://build.golang.org/log/51d557fdcbff67f0cb8e318be1a2c8c6e081c9f5

[rsc, 2013-07-29 19:32:56]

Try putting some #pragma textflag 7 in runtime/vlrt_arm.c.