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
https://golang.org/cl/180068

diff --git a/src/pkg/runtime/chan.c b/src/pkg/runtime/chan.c
index f0202cf66b5eba45c9ec06a61e0a838a41bfe409..b2a0b4facfc2976a54e43159b67a6db26d062f15 100644 (file)
--- a/src/pkg/runtime/chan.c
+++ b/src/pkg/runtime/chan.c
@@ -24,7 +24,7 @@ typedef       struct  Scase   Scase;
 struct SudoG
 {
        G*      g;              // g and selgen constitute
-       int32   selgen;         // a weak pointer to g
+       uint32  selgen;         // a weak pointer to g
        int16   offset;         // offset of case number
        int8    isfree;         // offset of case number
        SudoG*  link;
@@ -982,14 +982,12 @@ loop:
        q->first = sgp->link;
 
        // if sgp is stale, ignore it
-       if(sgp->selgen != sgp->g->selgen) {
+       if(!cas(&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;
 }
 

diff --git a/src/pkg/runtime/runtime.h b/src/pkg/runtime/runtime.h
index 91130a0052eaccc841996e2a86358d927b9a60b6..8052fd09ca3fbd3d46a47ce02f713f3ac40de357 100644 (file)
--- a/src/pkg/runtime/runtime.h
+++ b/src/pkg/runtime/runtime.h
@@ -167,7 +167,7 @@ struct      G
        void*   param;          // passed parameter on wakeup
        int16   status;
        int32   goid;
-       int32   selgen;         // valid sudog pointer
+       uint32  selgen;         // valid sudog pointer
        G*      schedlink;
        bool    readyonstop;
        M*      m;              // for debuggers, but offset not hard-coded

diff --git a/test/chan/doubleselect.go b/test/chan/doubleselect.go
new file mode 100644 (file)
index 0000000..53dafeb
--- /dev/null
+++ b/test/chan/doubleselect.go
@@ -0,0 +1,83 @@
+// $G $D/$F.go && $L $F.$A && ./$A.out
+
+// Copyright 2009 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This test is designed to flush out the case where two cases of a select can
+// both end up running. See http://codereview.appspot.com/180068.
+package main
+
+import (
+       "flag"
+       "runtime"
+)
+
+var iterations *int = flag.Int("n", 100000, "number of iterations")
+
+// sender sends a counter to one of four different channels. If two
+// cases both end up running in the same iteration, the same value will be sent
+// to two different channels.
+func sender(n int, c1, c2, c3, c4 chan<- int) {
+       defer close(c1)
+       defer close(c2)
+
+       for i := 0; i < n; i++ {
+               select {
+               case c1 <- i:
+               case c2 <- i:
+               case c3 <- i:
+               case c4 <- i:
+               }
+       }
+}
+
+// mux receives the values from sender and forwards them onto another channel.
+// It would be simplier to just have sender's four cases all be the same
+// channel, but this doesn't actually trigger the bug.
+func mux(out chan<- int, in <-chan int) {
+       for {
+               v := <-in
+               if closed(in) {
+                       close(out)
+                       break
+               }
+               out <- v
+       }
+}
+
+// recver gets a steam of values from the four mux's and checks for duplicates.
+func recver(in <-chan int) {
+       seen := make(map[int]bool)
+
+       for {
+               v := <-in
+               if closed(in) {
+                       break
+               }
+               if _, ok := seen[v]; ok {
+                       panic("got duplicate value: ", v)
+               }
+               seen[v] = true
+       }
+}
+
+func main() {
+       runtime.GOMAXPROCS(2)
+
+       c1 := make(chan int)
+       c2 := make(chan int)
+       c3 := make(chan int)
+       c4 := make(chan int)
+       cmux := make(chan int)
+       go sender(*iterations, c1, c2, c3, c4)
+       go mux(cmux, c1)
+       go mux(cmux, c2)
+       go mux(cmux, c3)
+       go mux(cmux, c4)
+       // We keep the recver because it might catch more bugs in the future.
+       // However, the result of the bug linked to at the top is that we'll
+       // end up panicing with: "throw: bad g->status in ready".
+       recver(cmux)
+       print("PASS\n")
+}

diff --git a/test/golden.out b/test/golden.out
index 9813c8313dcbc2b0ad022fc9499692e8bd77bc58..063feccd082c56507bf71d9cf9a651573dfc3a1e 100644 (file)
--- a/test/golden.out
+++ b/test/golden.out
@@ -75,6 +75,9 @@ abcxyz-abcxyz-abcxyz-abcxyz-abcxyz-abcxyz-abcxyz
 
 == chan/
 
+=========== chan/doubleselect.go
+PASS
+
 =========== chan/nonblock.go
 PASS