code review 159040043: runtime: dequeue the correct SudoG

runtime: dequeue the correct SudoG

select {
       case <- c:
       case <- c:
}

In this case, c.recvq lists two SudoGs which have the same G.
So we can't use the G as the key to dequeue the correct SudoG,
as that key is ambiguous.  Dequeueing the wrong SudoG ends up
freeing a SudoG that is still in c.recvq.

The fix is to use the actual SudoG pointer as the key.

[khr, 2014-10-18 00:24:02]

Hello rsc@golang.org (cc: austin@google.com,
golang-codereviews@googlegroups.com),

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

[bradfitz, 2014-10-18 09:43:48]

https://codereview.appspot.com/159040043/diff/70001/src/runtime/chan_test.go
File src/runtime/chan_test.go (right):

https://codereview.appspot.com/159040043/diff/70001/src/runtime/chan_test.go#...
src/runtime/chan_test.go:501: time.Sleep(time.Millisecond) // make sure
goroutine A gets qeueued first on c
the only risk of this being too short is failing to set up the conditions under
which it fails. I assume a number of our slower builders will fail to schedule
in a millisecond but we don't really care, right?

https://codereview.appspot.com/159040043/diff/70001/src/runtime/chan_test.go#...
src/runtime/chan_test.go:509: d <- 7 // wake up A, it dequeues itself from c and
corrupts c.recvq
corrupted

https://codereview.appspot.com/159040043/diff/70001/src/runtime/chan_test.go#...
src/runtime/chan_test.go:510: <-e    // A tells us he's done
it's

https://codereview.appspot.com/159040043/diff/70001/src/runtime/chan_test.go#...
src/runtime/chan_test.go:511: c <- 8 // try to wake up B, it will fail because
c.recvq is corrupt
it previously failed?

[dvyukov, 2014-10-18 15:29:54]

LGTM

[khr1, 2014-10-19 03:58:12]

On Sat, Oct 18, 2014 at 2:43 AM, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.com/159040043/diff/70001/src/
> runtime/chan_test.go
> File src/runtime/chan_test.go (right):
>
> https://codereview.appspot.com/159040043/diff/70001/src/
> runtime/chan_test.go#newcode501
> src/runtime/chan_test.go:501: time.Sleep(time.Millisecond) // make sure
> goroutine A gets qeueued first on c
> the only risk of this being too short is failing to set up the
> conditions under which it fails. I assume a number of our slower
> builders will fail to schedule in a millisecond but we don't really
> care, right?
>
>
That's about right.  It's standard practice in this file to give goroutines
a millisecond to start up and block, as there is way to detect successful
blocking directly.  If a slow platform doesn't get there, no big deal, the
fast platforms will still do the test correctly.


> https://codereview.appspot.com/159040043/diff/70001/src/
> runtime/chan_test.go#newcode509
> src/runtime/chan_test.go:509: d <- 7 // wake up A, it dequeues itself
> from c and corrupts c.recvq
> corrupted
>
> https://codereview.appspot.com/159040043/diff/70001/src/
> runtime/chan_test.go#newcode510
> src/runtime/chan_test.go:510: <-e    // A tells us he's done
> it's
>
> https://codereview.appspot.com/159040043/diff/70001/src/
> runtime/chan_test.go#newcode511
> src/runtime/chan_test.go:511: c <- 8 // try to wake up B, it will fail
> because c.recvq is corrupt
> it previously failed?
>
>
Right, I updated the language.


> https://codereview.appspot.com/159040043/
>

[khr1, 2014-10-19 03:58:37]

s/is way/is no way/

On Sat, Oct 18, 2014 at 8:58 PM, Keith Randall <khr@google.com> wrote:

>
>
> On Sat, Oct 18, 2014 at 2:43 AM, <bradfitz@golang.org> wrote:
>
>>
>> https://codereview.appspot.com/159040043/diff/70001/src/
>> runtime/chan_test.go
>> File src/runtime/chan_test.go (right):
>>
>> https://codereview.appspot.com/159040043/diff/70001/src/
>> runtime/chan_test.go#newcode501
>> src/runtime/chan_test.go:501: time.Sleep(time.Millisecond) // make sure
>> goroutine A gets qeueued first on c
>> the only risk of this being too short is failing to set up the
>> conditions under which it fails. I assume a number of our slower
>> builders will fail to schedule in a millisecond but we don't really
>> care, right?
>>
>>
> That's about right.  It's standard practice in this file to give
> goroutines a millisecond to start up and block, as there is way to detect
> successful blocking directly.  If a slow platform doesn't get there, no big
> deal, the fast platforms will still do the test correctly.
>
>
>> https://codereview.appspot.com/159040043/diff/70001/src/
>> runtime/chan_test.go#newcode509
>> src/runtime/chan_test.go:509: d <- 7 // wake up A, it dequeues itself
>> from c and corrupts c.recvq
>> corrupted
>>
>> https://codereview.appspot.com/159040043/diff/70001/src/
>> runtime/chan_test.go#newcode510
>> src/runtime/chan_test.go:510: <-e    // A tells us he's done
>> it's
>>
>> https://codereview.appspot.com/159040043/diff/70001/src/
>> runtime/chan_test.go#newcode511
>> src/runtime/chan_test.go:511: c <- 8 // try to wake up B, it will fail
>> because c.recvq is corrupt
>> it previously failed?
>>
>>
> Right, I updated the language.
>
>
>> https://codereview.appspot.com/159040043/
>>
>
>

[khr, 2014-10-19 04:02:55]

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

runtime: dequeue the correct SudoG

select {
       case <- c:
       case <- c:
}

In this case, c.recvq lists two SudoGs which have the same G.
So we can't use the G as the key to dequeue the correct SudoG,
as that key is ambiguous.  Dequeueing the wrong SudoG ends up
freeing a SudoG that is still in c.recvq.

The fix is to use the actual SudoG pointer as the key.

LGTM=dvyukov
R=rsc, bradfitz, dvyukov, khr
CC=austin, golang-codereviews
https://codereview.appspot.com/159040043

[rsc, 2014-10-20 15:01:29]

LGTM