code review 12418043: net: add special netFD mutex

net: add special netFD mutex
The mutex, fdMutex, handles locking and lifetime of sysfd,
and serializes Read and Write methods.
This allows to strip 2 sync.Mutex.Lock calls,
2 sync.Mutex.Unlock calls, 1 defer and some amount
of misc overhead from every network operation.

On linux/amd64, Intel E5-2690:
benchmark                             old ns/op    new ns/op    delta
BenchmarkTCP4Persistent                    9595         9454   -1.47%
BenchmarkTCP4Persistent-2                  8978         8772   -2.29%
BenchmarkTCP4ConcurrentReadWrite           4900         4625   -5.61%
BenchmarkTCP4ConcurrentReadWrite-2         2603         2500   -3.96%

In general it strips 70-500 ns from every network operation depending
on processor model. On my relatively new E5-2690 it accounts to ~5%
of network op cost.

Fixes issue 6074.

[dvyukov, 2013-08-06 18:32:59]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-08-06 18:35:53]

If we generally agree on this, then 2 refactorings can go separately (the first
is to expose runtime_Semacquire to net package, and another is to remove bool
parameter from incref). That will reduce number of changed files to ~4.

[dvyukov, 2013-08-06 19:11:18]

Just in case, if you are not sure where to start :) start at fd_mutex.go (the
mutex) and fd_unix.go/fd_windows.go (use of the mutex), everything else is
noise.

[bradfitz, 2013-08-06 19:13:45]

Is this worse on ARM, with the 64-bit atomics?



On Tue, Aug 6, 2013 at 12:11 PM, <dvyukov@google.com> wrote:

> Just in case, if you are not sure where to start :) start at fd_mutex.go
> (the mutex) and fd_unix.go/fd_windows.go (use of the mutex), everything
> else is noise.
>
>
>
>
https://codereview.appspot.**com/12418043/<https://codereview.appspot.com/124...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dvyukov, 2013-08-06 20:11:53]

On Tue, Aug 6, 2013 at 11:13 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Is this worse on ARM, with the 64-bit atomics?


I don't have ARM.
On the other hand it reduces number of atomic RMW operations per net
op from 6 to 2. So I would still expect it to be faster.
Theoretically it's possible to fit the state into 32 bits if necessary.

[brainman, 2013-08-07 00:02:17]

Sounds like a good idea to me. Makes all different OSes to behave the same.

Alex

[iant, 2013-08-07 00:45:26]

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go
File src/pkg/net/fd_mutex.go (right):

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:37: // Read operations must do ReadLock/ReadUnlock.
Why are these methods exported?  Will they ever be called by any other package?

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:38: // Write operations must do WriteLock/WriteUnlock.
Here you talk about ReadLock and WriteLock but below you only have RWLock.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:39: // Misc operations (setsockopt) must do Lock/Unlock.
I think it's a little confusing to have all of ReadLock and WriteLock and Lock,
especially since if I'm reading this correctly Lock is the one called least
often.  Should Lock be MiscLock?

Also it might be nice to expand on what misc operations are and why they need
their own lock.  read/write is pretty obvious, misc is not, at least not to me. 
And in fact it doesn't appear to be a lock at all, it appears to just manage a
reference count.  And the ReadLock and WriteLock names are a bit misleading
because they do not conflict with each other.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:42: // All lock operations return false if fd is closed.
These sentences are a little ambiguous.  Would it be correct to say:

// All lock operations return whether fd is open.
// All unlock operations return whether fd is closed and there are no remaining
references.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:68: new := old | mutexClosed + mutexRef
Please use parentheses here, we can't expect everyone to know the precedence of
| and +.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:124: // Read lock is free, acquire it.
s/Read lock/Lock/

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:125: new = old | mutexBit + mutexRef
Parentheses.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:130: // Wait for read lock.
s/read //

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:141: // The signaller has substracted mutexWait.
s/substracted/subtracted/

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:165: // Drop read lock, drop reference and wake read
waiter if present.
s/read //g

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:166: new := old&^mutexBit - mutexRef
Use parentheses.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex_test.go
File src/pkg/net/fd_mutex_test.go (right):

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex_test....
src/pkg/net/fd_mutex_test.go:82: case <-time.After(100 * time.Millisecond):
Increase this timeout so that we don't see pointless failures on slow machines.

[dvyukov, 2013-08-07 21:41:39]

PTAL

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go
File src/pkg/net/fd_mutex.go (right):

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:37: // Read operations must do ReadLock/ReadUnlock.
On 2013/08/07 00:45:27, iant wrote:
> Why are these methods exported?  Will they ever be called by any other
package?

But the type itself is not exported. How can a user call it?
netFD has lots of "exported" methods as well...

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:38: // Write operations must do WriteLock/WriteUnlock.
On 2013/08/07 00:45:27, iant wrote:
> Here you talk about ReadLock and WriteLock but below you only have RWLock.

comments tend to be outdate... there is nothing we can do with it :)
Done

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:39: // Misc operations (setsockopt) must do Lock/Unlock.
On 2013/08/07 00:45:27, iant wrote:
> I think it's a little confusing to have all of ReadLock and WriteLock and
Lock,
> especially since if I'm reading this correctly Lock is the one called least
> often.  Should Lock be MiscLock?
> 
> Also it might be nice to expand on what misc operations are and why they need
> their own lock.  read/write is pretty obvious, misc is not, at least not to
me. 
> And in fact it doesn't appear to be a lock at all, it appears to just manage a
> reference count.  And the ReadLock and WriteLock names are a bit misleading
> because they do not conflict with each other.

Agree
I will call them Incref/Decref, that's what they essentially are.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:42: // All lock operations return false if fd is closed.
On 2013/08/07 00:45:27, iant wrote:
> These sentences are a little ambiguous.  Would it be correct to say:
> 
> // All lock operations return whether fd is open.
> // All unlock operations return whether fd is closed and there are no
remaining
> references.

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:68: new := old | mutexClosed + mutexRef
On 2013/08/07 00:45:27, iant wrote:
> Please use parentheses here, we can't expect everyone to know the precedence
of
> | and +.

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:124: // Read lock is free, acquire it.
On 2013/08/07 00:45:27, iant wrote:
> s/Read lock/Lock/

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:125: new = old | mutexBit + mutexRef
On 2013/08/07 00:45:27, iant wrote:
> Parentheses.

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:130: // Wait for read lock.
On 2013/08/07 00:45:27, iant wrote:
> s/read //

Done
It's survived several refactorings already...

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:141: // The signaller has substracted mutexWait.
On 2013/08/07 00:45:27, iant wrote:
> s/substracted/subtracted/

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:165: // Drop read lock, drop reference and wake read
waiter if present.
On 2013/08/07 00:45:27, iant wrote:
> s/read //g

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:166: new := old&^mutexBit - mutexRef
On 2013/08/07 00:45:27, iant wrote:
> Use parentheses.

Done.

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex_test.go
File src/pkg/net/fd_mutex_test.go (right):

https://codereview.appspot.com/12418043/diff/27001/src/pkg/net/fd_mutex_test....
src/pkg/net/fd_mutex_test.go:82: case <-time.After(100 * time.Millisecond):
On 2013/08/07 00:45:27, iant wrote:
> Increase this timeout so that we don't see pointless failures on slow
machines.

Done.

[iant, 2013-08-09 00:01:26]

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_mutex.go
File src/pkg/net/fd_mutex.go (right):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:45: // All lock operations return false if fd is closed.
I think you can delete these two lines.

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:79: new &= ^uint64(mutexRMask | mutexWMask)
I think this can be
    new &^= mutexRMask | mutexWMask

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (left):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go#old...
src/pkg/net/fd_unix.go:87: fd.wio.Lock()
Why is it OK to just remove this lock?

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go#new...
src/pkg/net/fd_unix.go:108: if fd == nil || !fd.fdmu.Incref() {
Why do you need to test fd == nil?

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go#new...
src/pkg/net/fd_unix.go:117: if fd != nil && fd.fdmu.Decref() {
Why do you need to test fd != nil?

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go#new...
src/pkg/net/fd_unix.go:125: if fd == nil || !fd.fdmu.RWLock(true) {
Do you need check fd == nil here and below?

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (left):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:335: o.mu.Lock()
Why is it OK to get rid of this lock?

[mikio, 2013-08-09 04:35:26]

pls don't make sendfile_freebsd.go a black sheep.

[dvyukov, 2013-08-09 09:44:03]

PTAL

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_mutex.go
File src/pkg/net/fd_mutex.go (right):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:45: // All lock operations return false if fd is closed.
On 2013/08/09 00:01:27, iant wrote:
> I think you can delete these two lines.

Done.

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:79: new &= ^uint64(mutexRMask | mutexWMask)
On 2013/08/09 00:01:27, iant wrote:
> I think this can be
>     new &^= mutexRMask | mutexWMask

Done.

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (left):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go#old...
src/pkg/net/fd_unix.go:87: fd.wio.Lock()
On 2013/08/09 00:01:27, iant wrote:
> Why is it OK to just remove this lock?

the same reason as in fd_windows.go

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_unix.go#new...
src/pkg/net/fd_unix.go:108: if fd == nil || !fd.fdmu.Incref() {
On 2013/08/09 00:01:27, iant wrote:
> Why do you need to test fd == nil?

Windows does it, and I tried to unify them.
OK, I've removed the nil checks from both unix and windows, all net/... tests
pass on darwin and windows.

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (left):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:335: o.mu.Lock()
On 2013/08/09 00:01:27, iant wrote:
> Why is it OK to get rid of this lock?

netFD.connect is only called from netFD.dial, which is only called from socket
function. And the socket function also creates the netFD. So during connect
netFD is not yet accessible to user, and user can not do concurrent operations
on it. We return either already connected netFD or error.

[dvyukov, 2013-08-09 09:44:49]

On 2013/08/09 04:35:26, mikio wrote:
> pls don't make sendfile_freebsd.go a black sheep.

oops, it was unintentional, sorry
done

[iant, 2013-08-09 13:20:42]

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (left):

https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go#...
src/pkg/net/fd_windows.go:335: o.mu.Lock()
On 2013/08/09 09:44:04, dvyukov wrote:
> On 2013/08/09 00:01:27, iant wrote:
> > Why is it OK to get rid of this lock?
> 
> netFD.connect is only called from netFD.dial, which is only called from socket
> function. And the socket function also creates the netFD. So during connect
> netFD is not yet accessible to user, and user can not do concurrent operations
> on it. We return either already connected netFD or error.

Can you add a comment for this in the two relevant places?  Thanks.

[dvyukov, 2013-08-09 14:14:32]

On 2013/08/09 13:20:42, iant wrote:
> https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go
> File src/pkg/net/fd_windows.go (left):
> 
>
https://codereview.appspot.com/12418043/diff/53001/src/pkg/net/fd_windows.go#...
> src/pkg/net/fd_windows.go:335: o.mu.Lock()
> On 2013/08/09 09:44:04, dvyukov wrote:
> > On 2013/08/09 00:01:27, iant wrote:
> > > Why is it OK to get rid of this lock?
> > 
> > netFD.connect is only called from netFD.dial, which is only called from
socket
> > function. And the socket function also creates the netFD. So during connect
> > netFD is not yet accessible to user, and user can not do concurrent
operations
> > on it. We return either already connected netFD or error.
> 
> Can you add a comment for this in the two relevant places?  Thanks.


Done
PTAL

[iant, 2013-08-09 15:48:34]

LGTM

Thanks.

[dvyukov, 2013-08-09 16:10:51]

anybody else?

[bradfitz, 2013-08-09 16:26:53]

LGTM

It's now cleaner overall.

I don't see why the semacquire(profile false)  stuff is necessarily in this CL,
though. How is that related?

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go
File src/pkg/net/fd_mutex.go (right):

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:10: // that manages lifetime of fd and serializes access
of a fd (so it doesn't sound like fd is a field)

or "of an fd" (if you say it "eff-dee")

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:48: func (mu *fdMutex) Incref() bool {
document return value

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:64: func (mu *fdMutex) IncrefAndClose() bool {
document return value

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:93: func (mu *fdMutex) Decref() bool {
document return value

https://codereview.appspot.com/12418043/diff/77001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/12418043/diff/77001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:1024: void	runtime·semacquire(uint32*, bool);
can you name these? addr and profile?

or is that too long?

[dvyukov, 2013-08-09 17:05:58]

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go
File src/pkg/net/fd_mutex.go (right):

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:10: // that manages lifetime of fd and serializes access
On 2013/08/09 16:26:54, bradfitz wrote:
> of a fd (so it doesn't sound like fd is a field)
> 
> or "of an fd" (if you say it "eff-dee")

Done.

https://codereview.appspot.com/12418043/diff/77001/src/pkg/net/fd_mutex.go#ne...
src/pkg/net/fd_mutex.go:48: func (mu *fdMutex) Incref() bool {
On 2013/08/09 16:26:54, bradfitz wrote:
> document return value

the return values for all functions are documented on lines 45-46
do you think it's better to document it above each function?

https://codereview.appspot.com/12418043/diff/77001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/12418043/diff/77001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:1024: void	runtime·semacquire(uint32*, bool);
On 2013/08/09 16:26:54, bradfitz wrote:
> can you name these? addr and profile?
> 
> or is that too long?

most of the functions declared in runtime.h does not name parameters

[dvyukov, 2013-08-09 17:09:40]

On 2013/08/09 16:26:53, bradfitz wrote:
> LGTM
> 
> It's now cleaner overall.
> 
> I don't see why the semacquire(profile false)  stuff is necessarily in this
CL,
> though. How is that related?

Currently only runtime.semacquire(addr) is declared in runtime.h. And it's
intended for internal usage only, in particular they are not profiled with
blocking profiler. We want fdMutex in blocking profiler. So I had to refactor it
a bit and add runtime.semacquire(addr, profile) to runtime.h.

[bradfitz, 2013-08-09 17:13:39]

Gotcha.



On Fri, Aug 9, 2013 at 10:09 AM, <dvyukov@google.com> wrote:

> On 2013/08/09 16:26:53, bradfitz wrote:
>
>> LGTM
>>
>
>  It's now cleaner overall.
>>
>
>  I don't see why the semacquire(profile false)  stuff is necessarily in
>>
> this CL,
>
>> though. How is that related?
>>
>
> Currently only runtime.semacquire(addr) is declared in runtime.h. And
> it's intended for internal usage only, in particular they are not
> profiled with blocking profiler. We want fdMutex in blocking profiler.
> So I had to refactor it a bit and add runtime.semacquire(addr, profile)
> to runtime.h.
>
>
>
https://codereview.appspot.**com/12418043/<https://codereview.appspot.com/124...
>

[dvyukov, 2013-08-09 17:43:08]

*** Submitted as https://code.google.com/p/go/source/detail?r=7afd81b7fe12 ***

net: add special netFD mutex
The mutex, fdMutex, handles locking and lifetime of sysfd,
and serializes Read and Write methods.
This allows to strip 2 sync.Mutex.Lock calls,
2 sync.Mutex.Unlock calls, 1 defer and some amount
of misc overhead from every network operation.

On linux/amd64, Intel E5-2690:
benchmark                             old ns/op    new ns/op    delta
BenchmarkTCP4Persistent                    9595         9454   -1.47%
BenchmarkTCP4Persistent-2                  8978         8772   -2.29%
BenchmarkTCP4ConcurrentReadWrite           4900         4625   -5.61%
BenchmarkTCP4ConcurrentReadWrite-2         2603         2500   -3.96%

In general it strips 70-500 ns from every network operation depending
on processor model. On my relatively new E5-2690 it accounts to ~5%
of network op cost.

Fixes issue 6074.

R=golang-dev, bradfitz, alex.brainman, iant, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/12418043