From 30a68bfb806b5217932e280f5a5f521237e69077 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 27 Jul 2020 12:40:18 -0700 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Cuong Manh Le Reviewed-by: Keith Randall --- src/runtime/chan.go | 25 +++++++++++++++---------- src/runtime/runtime2.go | 6 ++++++ src/runtime/select.go | 19 +++++++------------ 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/runtime/chan.go b/src/runtime/chan.go index f6f4ffd02e..0afe5d962b 100644 --- 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 959878400d..b7d0739e54 100644 --- 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 a069e3e050..081db7bad4 100644 --- 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 { -- 2.50.0