code review 144940043: runtime: always run semacquire on the G stack

runtime: always run semacquire on the G stack

semacquire might need to park the currently running G.  It can
only park if called from the G stack (because it has no way of
saving the M stack state).  So all calls to semacquire must come
from the G stack.

The three violators are GOMAXPROCS, ReadMemStats, and WriteHeapDump.
This change moves the semacquire call earlier, out of their C code
and into their Go code.

This seldom caused bugs because semacquire seldom actually had
to park the caller.  But it did happen intermittently.

Fixes issue 8749

[khr, 2014-09-16 04:15:03]

Hello golang-codereviews@googlegroups.com,

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

[dvyukov, 2014-09-16 04:38:45]

https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c
File src/runtime/heapdump.c (left):

https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c#ol...
src/runtime/heapdump.c:765: g->m->gcing = 1;
I feel very nervous that you rearrange order of actions on worldsema, gcing,
locks and start/stoptheworld. I suspect that these are very magical sequences. I
don't want to look for exact cases that it breaks (e.g. trying to trigger GC
when worldsema is already acquired; or preempting the current goroutine before
it started the world -- I am very sure that I added locks++/-- around
starttheworld for a reason). Just preserve them verbatim.

[bradfitz, 2014-09-16 04:48:07]

On Tue, Sep 16, 2014 at 12:38 AM, dvyukov via golang-codereviews <
golang-codereviews@googlegroups.com> wrote:

>
> https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c
> File src/runtime/heapdump.c (left):
>
> https://codereview.appspot.com/144940043/diff/80001/src/
> runtime/heapdump.c#oldcode765
> src/runtime/heapdump.c:765: g->m->gcing = 1;
> I feel very nervous that you rearrange order of actions on worldsema,
> gcing, locks and start/stoptheworld. I suspect that these are very
> magical sequences. I don't want to look for exact cases that it breaks
>

That's what tests are for. :-)


> (e.g. trying to trigger GC when worldsema is already acquired; or
> preempting the current goroutine before it started the world -- I am
> very sure that I added locks++/-- around starttheworld for a reason).
> Just preserve them verbatim.

[dvyukov, 2014-09-16 04:51:49]

On Mon, Sep 15, 2014 at 9:48 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
> On Tue, Sep 16, 2014 at 12:38 AM, dvyukov via golang-codereviews
> <golang-codereviews@googlegroups.com> wrote:
>>
>>
>> https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c
>> File src/runtime/heapdump.c (left):
>>
>>
>>
https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c#ol...
>> src/runtime/heapdump.c:765: g->m->gcing = 1;
>> I feel very nervous that you rearrange order of actions on worldsema,
>> gcing, locks and start/stoptheworld. I suspect that these are very
>> magical sequences. I don't want to look for exact cases that it breaks
>
>
> That's what tests are for. :-)


It's impossible to test preemption or GC triggering in a very
particular point in runtime code.

[bradfitz, 2014-09-16 04:52:55]

On Tue, Sep 16, 2014 at 12:51 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Mon, Sep 15, 2014 at 9:48 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> >
> >
> > On Tue, Sep 16, 2014 at 12:38 AM, dvyukov via golang-codereviews
> > <golang-codereviews@googlegroups.com> wrote:
> >>
> >>
> >>
> https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c
> >> File src/runtime/heapdump.c (left):
> >>
> >>
> >>
>
https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c#ol...
> >> src/runtime/heapdump.c:765: g->m->gcing = 1;
> >> I feel very nervous that you rearrange order of actions on worldsema,
> >> gcing, locks and start/stoptheworld. I suspect that these are very
> >> magical sequences. I don't want to look for exact cases that it breaks
> >
> >
> > That's what tests are for. :-)
>
>
> It's impossible to test preemption or GC triggering in a very
> particular point in runtime code.
>

There's nothing impossible about it. We just don't have tests for it.

[khr, 2014-09-16 20:47:00]

On 2014/09/16 04:52:55, bradfitz wrote:
> On Tue, Sep 16, 2014 at 12:51 AM, Dmitry Vyukov <mailto:dvyukov@google.com>
wrote:
> 
> > On Mon, Sep 15, 2014 at 9:48 PM, Brad Fitzpatrick
<mailto:bradfitz@golang.org>
> > wrote:
> > >
> > >
> > > On Tue, Sep 16, 2014 at 12:38 AM, dvyukov via golang-codereviews
> > > <mailto:golang-codereviews@googlegroups.com> wrote:
> > >>
> > >>
> > >>
> > https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c
> > >> File src/runtime/heapdump.c (left):
> > >>
> > >>
> > >>
> >
>
https://codereview.appspot.com/144940043/diff/80001/src/runtime/heapdump.c#ol...
> > >> src/runtime/heapdump.c:765: g->m->gcing = 1;
> > >> I feel very nervous that you rearrange order of actions on worldsema,
> > >> gcing, locks and start/stoptheworld. I suspect that these are very
> > >> magical sequences. I don't want to look for exact cases that it breaks
> > >
> > >
> > > That's what tests are for. :-)
> >
> >
> > It's impossible to test preemption or GC triggering in a very
> > particular point in runtime code.
> >
> 
> There's nothing impossible about it. We just don't have tests for it.

PTAL.

I've restored the ordering to match the original C code.  I'm not very happy
about inscrutable magic code, but Dmitry is right, at this point in the release
cycle we're better off not risking it.  I definitely want to revisit some of
this for 1.5.  For a start, "gcing" is certainly the wrong name for whatever
we're trying to describe.

[dvyukov, 2014-09-17 00:18:30]

https://codereview.appspot.com/144940043/diff/120001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.appspot.com/144940043/diff/120001/src/runtime/runtime.h#ne...
src/runtime/runtime.h:437: struct SchedType {
{ on next line

https://codereview.appspot.com/144940043/diff/120001/src/runtime/sema.go
File src/runtime/sema.go (right):

https://codereview.appspot.com/144940043/diff/120001/src/runtime/sema.go#newc...
src/runtime/sema.go:53: if g != g.m.curg {
if gp := getg(); gp != gp.m.curg {

we used to call it 'gp', 'g' is tls register in C and type in Go

[dvyukov, 2014-09-17 00:18:39]

LGTM otherwise

[khr, 2014-09-17 00:26:19]

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

runtime: always run semacquire on the G stack

semacquire might need to park the currently running G.  It can
only park if called from the G stack (because it has no way of
saving the M stack state).  So all calls to semacquire must come
from the G stack.

The three violators are GOMAXPROCS, ReadMemStats, and WriteHeapDump.
This change moves the semacquire call earlier, out of their C code
and into their Go code.

This seldom caused bugs because semacquire seldom actually had
to park the caller.  But it did happen intermittently.

Fixes issue 8749

LGTM=dvyukov
R=golang-codereviews, dvyukov, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/144940043