runtime: fix race condition

runtime: fix race condition

(Thanks to ken and rsc for pointing this out)

rsc:
	ken pointed out that there's a race in the new
	one-lock-per-channel code.  the issue is that
	if one goroutine has gone to sleep doing

	select {
	case <-c1:
	case <-c2:
	}

	and then two more goroutines try to send
	on c1 and c2 simultaneously, the way that
	the code makes sure only one wins is the
	selgen field manipulation in dequeue:

	       // if sgp is stale, ignore it
	       if(sgp->selgen != sgp->g->selgen) {
		       //prints("INVALID PSEUDOG POINTER\n");
		       freesg(c, sgp);
		       goto loop;
	       }

	       // invalidate any others
	       sgp->g->selgen++;

	but because the global lock is gone both
	goroutines will be fiddling with sgp->g->selgen
	at the same time.

This results in a 7% slowdown in the single threaded case for a
ping-pong microbenchmark.

Since the cas predominantly succeeds, adding a simple check first
didn't make any difference.

[rsc, 2009-12-16 21:28:19]

LGTM

but please make selgen a uint32 so that you don't need the cast.

On Wed, Dec 16, 2009 at 13:00,  <agl@golang.org> wrote:
> Reviewers: rsc,
>
> Description:
> runtime: fix race condition
>
> (Thanks to ken and rsc for pointing this out)
>
> rsc:
>        ken pointed out that there's a race in the new
>        one-lock-per-channel code.  the issue is that
>        if one goroutine has gone to sleep doing
>
>        select {
>        case <-c1:
>        case <-c2:
>        }
>
>        and then two more goroutines try to send
>        on c1 and c2 simultaneously, the way that
>        the code makes sure only one wins is the
>        selgen field manipulation in dequeue:
>
>               // if sgp is stale, ignore it
>               if(sgp->selgen != sgp->g->selgen) {
>                       //prints("INVALID PSEUDOG POINTER\n");
>                       freesg(c, sgp);
>                       goto loop;
>               }
>
>               // invalidate any others
>               sgp->g->selgen++;
>
>        but because the global lock is gone both
>        goroutines will be fiddling with sgp->g->selgen
>        at the same time.
>
> This results in a 7% slowdown in the single threaded case for a
> ping-pong microbenchmark.
>
> Since the cas predominantly succeeds, adding a simple check first
> didn't make any difference.
>
> Please review this at http://codereview.appspot.com/180068
>
> Affected files:
>  M src/pkg/runtime/chan.c
>
>
> Index: src/pkg/runtime/chan.c
> ===================================================================
> --- a/src/pkg/runtime/chan.c
> +++ b/src/pkg/runtime/chan.c
> @@ -982,14 +982,12 @@
>        q->first = sgp->link;
>
>        // if sgp is stale, ignore it
> -       if(sgp->selgen != sgp->g->selgen) {
> +       if(!cas((uint32*)&sgp->g->selgen, sgp->selgen, sgp->selgen + 1)) {
>                //prints("INVALID PSEUDOG POINTER\n");
>                freesg(c, sgp);
>                goto loop;
>        }
>
> -       // invalidate any others
> -       sgp->g->selgen++;
>        return sgp;
>  }
>
>
>
>

[ken3, 2009-12-16 21:32:57]

i am not sure that this fixes the race.
please come see me.

On Wednesday, December 16, 2009,  <agl@golang.org> wrote:
> Reviewers: rsc,
>
> Description:
> runtime: fix race condition
>
> (Thanks to ken and rsc for pointing this out)
>
> rsc:
>         ken pointed out that there's a race in the new
>         one-lock-per-channel code.  the issue is that
>         if one goroutine has gone to sleep doing
>
>         select {
>         case <-c1:
>         case <-c2:
>         }
>
>         and then two more goroutines try to send
>         on c1 and c2 simultaneously, the way that
>         the code makes sure only one wins is the
>         selgen field manipulation in dequeue:
>
>                // if sgp is stale, ignore it
>                if(sgp->selgen != sgp->g->selgen) {
>                        //prints("INVALID PSEUDOG POINTER\n");
>                        freesg(c, sgp);
>                        goto loop;
>                }
>
>                // invalidate any others
>                sgp->g->selgen++;
>
>         but because the global lock is gone both
>         goroutines will be fiddling with sgp->g->selgen
>         at the same time.
>
> This results in a 7% slowdown in the single threaded case for a
> ping-pong microbenchmark.
>
> Since the cas predominantly succeeds, adding a simple check first
> didn't make any difference.
>
> Please review this at http://codereview.appspot.com/180068
>
> Affected files:
>   M src/pkg/runtime/chan.c
>
>
> Index: src/pkg/runtime/chan.c
> ===================================================================
> --- a/src/pkg/runtime/chan.c
> +++ b/src/pkg/runtime/chan.c
> @@ -982,14 +982,12 @@
>         q->first = sgp->link;
>
>         // if sgp is stale, ignore it
> -       if(sgp->selgen != sgp->g->selgen) {
> +       if(!cas((uint32*)&sgp->g->selgen, sgp->selgen, sgp->selgen + 1)) {
>                 //prints("INVALID PSEUDOG POINTER\n");
>                 freesg(c, sgp);
>                 goto loop;
>         }
>
> -       // invalidate any others
> -       sgp->g->selgen++;
>         return sgp;
>  }
>
>
>
>

[agl1, 2009-12-16 23:01:58]

PTAL

[rsc, 2009-12-18 07:03:23]

LGTM

[agl1, 2009-12-18 20:25:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=2ccb1970a8a5 ***

runtime: fix race condition

(Thanks to ken and rsc for pointing this out)

rsc:
	ken pointed out that there's a race in the new
	one-lock-per-channel code.  the issue is that
	if one goroutine has gone to sleep doing

	select {
	case <-c1:
	case <-c2:
	}

	and then two more goroutines try to send
	on c1 and c2 simultaneously, the way that
	the code makes sure only one wins is the
	selgen field manipulation in dequeue:

	       // if sgp is stale, ignore it
	       if(sgp->selgen != sgp->g->selgen) {
		       //prints("INVALID PSEUDOG POINTER\n");
		       freesg(c, sgp);
		       goto loop;
	       }

	       // invalidate any others
	       sgp->g->selgen++;

	but because the global lock is gone both
	goroutines will be fiddling with sgp->g->selgen
	at the same time.

This results in a 7% slowdown in the single threaded case for a
ping-pong microbenchmark.

Since the cas predominantly succeeds, adding a simple check first
didn't make any difference.

R=rsc
CC=golang-dev
http://codereview.appspot.com/180068