runtime: add "success" field to sudog

The current wakeup protocol for channel communications is that the
second goroutine sets gp.param to the sudog when a value is
successfully communicated over the channel, and to nil when the wakeup
is due to closing the channel.

Setting nil to indicate channel closure works okay for chansend and
chanrecv, because they're only communicating with one channel, so they
know it must be the channel that was closed. However, it means
selectgo has to re-poll all of the channels to figure out which one
was closed.

This commit adds a "success" field to sudog, and changes the wakeup
protocol to always set gp.param to sg, and to use sg.success to
indicate successful communication vs channel closure.

While here, this also reorganizes the chansend code slightly so that
the sudog is still released to the pool if the send blocks and then is
awoken because the channel closed.

Updates #40410.

Change-Id: I6cd9a20ebf9febe370a15af1b8afe24c5539efc6
Reviewed-on: https://go-review.googlesource.com/c/go/+/245019
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/runtime/chan.go b/src/runtime/chan.go
index f6f4ffd02e1d21597c533b814224aea0612252d0..0afe5d962bfbabef70434e4bf5a1d508f43efd4f 100644 (file)
--- a/src/runtime/chan.go
+++ b/src/runtime/chan.go
@@ -263,18 +263,19 @@ func chansend(c *hchan, ep unsafe.Pointer, block bool, callerpc uintptr) bool {
        }
        gp.waiting = nil
        gp.activeStackChans = false
-       if gp.param == nil {
-               if c.closed == 0 {
-                       throw("chansend: spurious wakeup")
-               }
-               panic(plainError("send on closed channel"))
-       }
+       closed := !mysg.success
        gp.param = nil
        if mysg.releasetime > 0 {
                blockevent(mysg.releasetime-t0, 2)
        }
        mysg.c = nil
        releaseSudog(mysg)
+       if closed {
+               if c.closed == 0 {
+                       throw("chansend: spurious wakeup")
+               }
+               panic(plainError("send on closed channel"))
+       }
        return true
 }
 
@@ -311,6 +312,7 @@ func send(c *hchan, sg *sudog, ep unsafe.Pointer, unlockf func(), skip int) {
        gp := sg.g
        unlockf()
        gp.param = unsafe.Pointer(sg)
+       sg.success = true
        if sg.releasetime != 0 {
                sg.releasetime = cputicks()
        }
@@ -384,7 +386,8 @@ func closechan(c *hchan) {
                        sg.releasetime = cputicks()
                }
                gp := sg.g
-               gp.param = nil
+               gp.param = unsafe.Pointer(sg)
+               sg.success = false
                if raceenabled {
                        raceacquireg(gp, c.raceaddr())
                }
@@ -402,7 +405,8 @@ func closechan(c *hchan) {
                        sg.releasetime = cputicks()
                }
                gp := sg.g
-               gp.param = nil
+               gp.param = unsafe.Pointer(sg)
+               sg.success = false
                if raceenabled {
                        raceacquireg(gp, c.raceaddr())
                }
@@ -575,11 +579,11 @@ func chanrecv(c *hchan, ep unsafe.Pointer, block bool) (selected, received bool)
        if mysg.releasetime > 0 {
                blockevent(mysg.releasetime-t0, 2)
        }
-       closed := gp.param == nil
+       success := mysg.success
        gp.param = nil
        mysg.c = nil
        releaseSudog(mysg)
-       return true, !closed
+       return true, success
 }
 
 // recv processes a receive operation on a full channel c.
@@ -632,6 +636,7 @@ func recv(c *hchan, sg *sudog, ep unsafe.Pointer, unlockf func(), skip int) {
        gp := sg.g
        unlockf()
        gp.param = unsafe.Pointer(sg)
+       sg.success = true
        if sg.releasetime != 0 {
                sg.releasetime = cputicks()
        }

diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index 959878400dda3ba3831e0475af279f6b180e1aac..b7d0739e543e3984c14ffc9287fc00ad57f12c6b 100644 (file)
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -366,6 +366,12 @@ type sudog struct {
        // g.selectDone must be CAS'd to win the wake-up race.
        isSelect bool
 
+       // success indicates whether communication over channel c
+       // succeeded. It is true if the goroutine was awoken because a
+       // value was delivered over channel c, and false if awoken
+       // because c was closed.
+       success bool
+
        parent   *sudog // semaRoot binary tree
        waitlink *sudog // g.waiting list or semaRoot
        waittail *sudog // semaRoot

diff --git a/src/runtime/select.go b/src/runtime/select.go
index a069e3e0502dca32d76448309efe07f590f2b62f..081db7bad4e667a7054aa8c9fc7c2ecb0f47f6ad 100644 (file)
--- a/src/runtime/select.go
+++ b/src/runtime/select.go
@@ -221,12 +221,12 @@ func selectgo(cas0 *scase, order0 *uint16, ncases int) (int, bool) {
                nextp  **sudog
        )
 
-loop:
        // pass 1 - look for something already waiting
        var dfli int
        var dfl *scase
        var casi int
        var cas *scase
+       var caseSuccess bool
        var recvOK bool
        for i := 0; i < ncases; i++ {
                casi = int(pollorder[i])
@@ -331,6 +331,7 @@ loop:
        // We singly-linked up the SudoGs in lock order.
        casi = -1
        cas = nil
+       caseSuccess = false
        sglist = gp.waiting
        // Clear all elem before unlinking from gp.waiting.
        for sg1 := gp.waiting; sg1 != nil; sg1 = sg1.waitlink {
@@ -352,6 +353,7 @@ loop:
                        // sg has already been dequeued by the G that woke us up.
                        casi = int(casei)
                        cas = k
+                       caseSuccess = sglist.success
                } else {
                        c = k.c
                        if k.kind == caseSend {
@@ -367,16 +369,7 @@ loop:
        }
 
        if cas == nil {
-               // We can wake up with gp.param == nil (so cas == nil)
-               // when a channel involved in the select has been closed.
-               // It is easiest to loop and re-run the operation;
-               // we'll see that it's now closed.
-               // Maybe some day we can signal the close explicitly,
-               // but we'd have to distinguish close-on-reader from close-on-writer.
-               // It's easiest not to duplicate the code and just recheck above.
-               // We know that something closed, and things never un-close,
-               // so we won't block again.
-               goto loop
+               throw("selectgo: bad wakeup")
        }
 
        c = cas.c
@@ -386,7 +379,9 @@ loop:
        }
 
        if cas.kind == caseRecv {
-               recvOK = true
+               recvOK = caseSuccess
+       } else if cas.kind == caseSend && !caseSuccess {
+               goto sclose
        }
 
        if raceenabled {