code review 185780043: runtime: simplify lock-free logic in netpoll

runtime: simplify lock-free logic in netpoll

Before, pd.closing, pd.rt/pd.wt, and pd.rg/pd.wg were all
accessed in a lock-free manner. In this CL, the bits being
checked in pd.closing and pd.rt/pd.wt are moved into the
low bits of pd.rg/pd.wg, so that there is only one memory
word being accessed lock-free.

Austin, Rick, and I spent two hours at a wall-sized whiteboard
covered with the relevant flow of the existing code this morning.
We were unable to find a flaw but also unable to convince ourselves
it is correct. Having just one lock-free word (one for reader and
one for writer, but those are independent) should be much easier
to reason about, and it may actually be correct.

This change is for Go 1.5, not Go 1.4.

(If adding the Go 1.2 locks back to the current implementation
fixes the problems we are seeing, we will ship Go 1.4 with locks.)

[rsc, 2014-12-02 21:50:43]

Hello austin, dvyukov, rlh (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[austin, 2014-12-03 04:16:36]

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go
File src/runtime/netpoll.go (left):

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ol...
src/runtime/netpoll.go:140: err := netpollcheckerr(pd, int32(mode))
Did netpollarm need this check?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go
File src/runtime/netpoll.go (right):

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:30: rt timer	// read deadline timer (set if rt.f != nil,
rt > 0)
rd > 0 (instead of rt > 0)?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:32: wt timer	// write deadline timer (set if wt.f != nil,
wt > 0)
Ditto.

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:37: // OR'ed into the low bits.
I take it Gs are 8 byte aligned, even on 32-bit?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:88: atomicstoreuintptr(&pd.rg, 0)
Now you're just being paranoid. :)

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:102: lock(&pd.lock)
Given that we're about to free pd, I hope this lock is just belt-and-suspenders.

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:111: atomicstoreuintptr(&pd.rg, 0)
Nit: I would swap the atomicstoreuintptr with the if wg, just so the checks are
all grouped and the code continues its (rg, wg)+ cadence.

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:121: println("pd", pd.rt.f, pd.rd, pd.wt.f, pd.wd)
Left over debug print?  This isn't in the rt case.

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:192: netpollblock(pdg, pdStatusReady)
Is it okay that this loop is gone and we ignore the result of netpollblock?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:195: func netpollremovetimers(pd *pollDesc) {
Add a comment that this must be called with pd.lock held

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:195: func netpollremovetimers(pd *pollDesc) {
camelCase?  Or does this mean this is "more private"?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:210: func netpollclear(pdg *uintptr, status uintptr) {
camelCase?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:232: if d != 0 && d <= nanotime() {
This seems sketchy (but may be fine).  What if d becomes in the past before we
addtimer?  Is this just an optimization to avoid going through timers when it's
obviously in the past?

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:297: }
Looks good to here (admittedly the interesting stuff is below).  I'll pick back
up tomorrow.

[dvyukov, 2014-12-03 08:10:21]

Have you taken windows into account? It has significantly different logic. I see
that you say that if we return from netpollWait, we will just retry Read. This
is not true on windows. On windows we issue async operation once, and then when
netpollWait returns we consider the operation as completed or cancel it. There
is no retry loop. Also netpollblock has some subtle logic wrt cancellation
(waitio arg).

[rlh, 2014-12-03 15:39:20]

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go
File src/runtime/netpoll.go (right):

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:28: lock mutex
Not sure what this lock is intended to protect and when. It is taken at times
and then I see atomic loads and stores of what I thought it was protecting.
Perhaps the atomics are just remnants of the non-blocking version but they imply
the lock isn't all you need. Austin mentions this as "belt and suspenders"
below.

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:313: }
Where are netpolllock and netpollunlock used, is it left over debug code? If
they are being used they probably should be used everywhere instead of just the
raw lock(&pd.lock) and unlock(&pd.lock).

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:336: // a network error value (netErrClosing,
netErrTimeout),
Formatting seems strange.

[dvyukov, 2014-12-03 16:14:13]

I do not see this as a net win. We are exchanging one complex lock-free code to
another complex lock-free code. For this new code, we are even less sure that it
is correct. Also it uses tons of atomic RMW operations where necessary and where
it is not necessary. The whole point of lock removal was performance, and I
suspect this version is slower than the original lock-based version.

[mattn, 2014-12-03 16:34:53]

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go
File src/runtime/netpoll.go (right):

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:366: _g_.iowait = status // can use closure in Go 1.5
iowait is not provided on windows. netpollblock is used in common.

[rsc, 2014-12-03 16:52:35]

On Wed, Dec 3, 2014 at 11:14 AM, <dvyukov@google.com> wrote:

> I do not see this as a net win. We are exchanging one complex lock-free
> code to another complex lock-free code. For this new code, we are even
> less sure that it is correct. Also it uses tons of atomic RMW operations
> where necessary and where it is not necessary. The whole point of lock
> removal was performance, and I suspect this version is slower than the
> original lock-based version.
>

We need to have code that people other than you can understand and reason
about. Austin, Rick, and I spent hours at the whiteboard yesterday trying
to convince ourselves one way or the other whether the multiple-words
version was correct, and we could not (either way). Today I think it's
probably correct, but it's just too complicated.

Conceptually, all this does is merge the two bits pd.closing == true and
pd.rt < 0 into pd.rg. That gets us down to one word, which is easier to
reason about. Maybe there are atomics here that can be relaxed, maybe
Windows needs work, and so on, but I see no reason we fundamentally need to
be doing lock-free accesses to 3 words when we can do 1 word instead.

Russ

[austin, 2014-12-03 17:16:39]

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go
File src/runtime/netpoll.go (right):

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:308: return atomicloaduintptr(&pd.rg) & pdStatusClosing
!= 0
This may be racy, but I don't see any uses of netpollclosing.

https://codereview.appspot.com/185780043/diff/40001/src/runtime/netpoll.go#ne...
src/runtime/netpoll.go:365: _g_ := getg()
Could gothrow if old&^pdStatusMask != 0 like the old code does.

[dvyukov, 2014-12-11 16:25:18]

On Wed, Dec 3, 2014 at 5:52 PM, Russ Cox <rsc@golang.org> wrote:
> On Wed, Dec 3, 2014 at 11:14 AM, <dvyukov@google.com> wrote:
>>
>> I do not see this as a net win. We are exchanging one complex lock-free
>> code to another complex lock-free code. For this new code, we are even
>> less sure that it is correct. Also it uses tons of atomic RMW operations
>> where necessary and where it is not necessary. The whole point of lock
>> removal was performance, and I suspect this version is slower than the
>> original lock-based version.
>
>
> We need to have code that people other than you can understand and reason
> about. Austin, Rick, and I spent hours at the whiteboard yesterday trying to
> convince ourselves one way or the other whether the multiple-words version
> was correct, and we could not (either way). Today I think it's probably
> correct, but it's just too complicated.
>
> Conceptually, all this does is merge the two bits pd.closing == true and
> pd.rt < 0 into pd.rg. That gets us down to one word, which is easier to
> reason about. Maybe there are atomics here that can be relaxed, maybe
> Windows needs work, and so on, but I see no reason we fundamentally need to
> be doing lock-free accesses to 3 words when we can do 1 word instead.


Does this fix the hangs that we see?

[gobot, 2014-12-19 05:17:24]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/185780043 is best) in the description in your
new CL.

Thanks very much.