Code review - Issue 8608044: code review 8608044: net: separate pollster initialization from network file descriptor allocationhttps://codereview.appspot.com/2013-08-06T14:42:41+00:00rietveld
Message from bradfitz@golang.org
2013-06-05T21:04:42+00:00bradfitzurn:md5:5ae3edfdd51a400666504bc9dfb15e0f
What's the status of this?
Message from mikioh.mikioh@gmail.com
2013-06-05T21:36:34+00:00mikiourn:md5:affd5fc5baad158a882f9d8be72cad98
On Thu, Jun 6, 2013 at 6:04 AM, <bradfitz@golang.org> wrote:
> What's the status of this?
It's part of issue 5199.
https://groups.google.com/d/msg/golang-dev/hkygwgxDQ-s/HUQJGAomOksJ
Message from iant@golang.org
2013-06-20T16:33:47+00:00ianturn:md5:31b5ee8265edd78057a0ec09d285f99b
R=dvyukov
Message from dvyukov@google.com
2013-06-22T15:51:11+00:00dvyukovurn:md5:7200116a4885bbdad3e5d10ee13fceaf
Is it ready for review? It seems to be not mailed.
Message from dvyukov@google.com
2013-06-24T11:18:15+00:00dvyukovurn:md5:1e2950d420f538cc7d09b8469263e659
On 2013/06/22 15:51:11, dvyukov wrote:
> Is it ready for review? It seems to be not mailed.
Q=wait
Message from dvyukov@google.com
2013-08-01T12:33:45+00:00dvyukovurn:md5:45a4b24d57f0fc642015b9fed3d8da9c
Is this still necessary? I thought that bsd will reuse darwin code, which does not need the change.
Message from mikioh.mikioh@gmail.com
2013-08-01T13:05:41+00:00mikiourn:md5:a4181fcaee712fdf47b6cd2887434f50
On Thu, Aug 1, 2013 at 9:33 PM, <dvyukov@google.com> wrote:
> Is this still necessary? I thought that bsd will reuse darwin code,
> which does not need the change.
I think, we need to pass a socket that has been called listen
previously to kqueue on BSD variants for stream listeners
rather than using NOTE_LOWAT, overriding SO_RCVLOWAT
to pull up pending connections from the kernel.
Not sure the reason why Darwin works well so far.
Am I missing something?
Message from dvyukov@google.com
2013-08-01T13:11:27+00:00dvyukovurn:md5:2502f92e56011878191c109de9921427
On 2013/08/01 13:05:41, mikio wrote:
> On Thu, Aug 1, 2013 at 9:33 PM, <mailto:dvyukov@google.com> wrote:
>
> > Is this still necessary? I thought that bsd will reuse darwin code,
> > which does not need the change.
>
> I think, we need to pass a socket that has been called listen
> previously to kqueue on BSD variants for stream listeners
> rather than using NOTE_LOWAT, overriding SO_RCVLOWAT
> to pull up pending connections from the kernel.
>
> Not sure the reason why Darwin works well so far.
>
> Am I missing something?
I don't know what is necessary for bsd systems. Darwin is happy with current code.
Message from mikioh.mikioh@gmail.com
2013-08-01T13:22:44+00:00mikiourn:md5:49723790d8c579e4f14ae89e05e03ed2
On Thu, Aug 1, 2013 at 10:11 PM, <dvyukov@google.com> wrote:
> I don't know what is necessary for bsd systems. Darwin is happy with
> current code.
-- kqueue on-line manual --
EVFILT_READ
(snip)
Sockets
Sockets which have previously been passed to listen()
return when there is an incoming connection pending.
data contains the size of the listen backlog.
Other socket descriptors return when there is data to
be read, subject to the SO_RCVLOWAT value of the
socket buffer. This may be overridden with a per-fil-
ter low water mark at the time the filter is added by
setting the NOTE_LOWAT flag in fflags, and specifying
the new low water mark in data. On return, data con-
tains the number of bytes of protocol data available
to read.
--
hmm...
Message from dvyukov@google.com
2013-08-01T13:33:07+00:00dvyukovurn:md5:9eefcaf90a6c48efd499ad9a88667537
I would expect to just get an edge-triggered read notification per
each new incoming connection. Is not it true on bsd systems?
On Thu, Aug 1, 2013 at 5:22 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Thu, Aug 1, 2013 at 10:11 PM, <dvyukov@google.com> wrote:
>
>> I don't know what is necessary for bsd systems. Darwin is happy with
>> current code.
>
> -- kqueue on-line manual --
> EVFILT_READ
>
> (snip)
>
> Sockets
> Sockets which have previously been passed to listen()
> return when there is an incoming connection pending.
> data contains the size of the listen backlog.
>
> Other socket descriptors return when there is data to
> be read, subject to the SO_RCVLOWAT value of the
> socket buffer. This may be overridden with a per-fil-
> ter low water mark at the time the filter is added by
> setting the NOTE_LOWAT flag in fflags, and specifying
> the new low water mark in data. On return, data con-
> tains the number of bytes of protocol data available
> to read.
> --
>
> hmm...
Message from mikioh.mikioh@gmail.com
2013-08-01T23:50:15+00:00mikiourn:md5:16fd69f408ea91f9291eb094e3e27e68
On Thu, Aug 1, 2013 at 10:32 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> I would expect to just get an edge-triggered read notification per
> each new incoming connection. Is not it true on bsd systems?
Looks like. I'm guessing the reason why Darwin works well so far
is because Darwin is kinda chimera; capturing incoming something
at Mach I/O stuff and notifying to BSD subsystem probably make
some data to be read. Just guessing.
Message from unknown
2013-08-05T01:34:44+00:00mikiourn:md5:8e369f1a75269c2818f5c88774c14e57
Message from unknown
2013-08-05T02:07:44+00:00mikiourn:md5:e715e89d1706b74c2c11affc9e175f33
Message from mikioh.mikioh@gmail.com
2013-08-05T02:07:54+00:00mikiourn:md5:df46fe8551b0fdec9c0f702f4ad68544
Hello dvyukov@google.com, alex.brainman@gmail.com, minux.ma@gmail.com (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from dvyukov@google.com
2013-08-05T08:40:57+00:00dvyukovurn:md5:cbdc1d6acc4e2b42f8f59c997d7445fb
Does not BSD work w/o this change?
The documentation snippet can be interpreted in two ways: (1) you must call listen before associating the fd with kqueue, or (2) you may associate fd with kqueue before listen, but will receive notifications on the fd only after listen call.
I would expect it to be (2).
If it is (1), then I would suggest to call current all-in-one runtime_pollInit after listen. Because after the split runtime_pollInit becomes senseless operation, there is no need to call it before runtime_pollOpen.
Message from mikioh.mikioh@gmail.com
2013-08-05T11:47:16+00:00mikiourn:md5:9eef790d75124084fededafaa4de4db4
On Mon, Aug 5, 2013 at 5:40 PM, <dvyukov@google.com> wrote:
> Does not BSD work w/o this change?
Just did a dumb experiment on FreeBSD 9.1;
<https://gist.github.com/mikioh/6155250>
ListenAfterKevent didn't work well.
> If it is (1), then I would suggest to call current all-in-one
> runtime_pollInit after listen. Because after the split runtime_pollInit
> becomes senseless operation, there is no need to call it before
> runtime_pollOpen.
Sounds reasonable to me. Alex, do you agree with Dmitry's proposal?
Message from dvyukov@google.com
2013-08-05T11:59:36+00:00dvyukovurn:md5:5bdf04d1037fb9916bf22ffaeca3193d
On Mon, Aug 5, 2013 at 3:47 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Mon, Aug 5, 2013 at 5:40 PM, <dvyukov@google.com> wrote:
>
>> Does not BSD work w/o this change?
>
> Just did a dumb experiment on FreeBSD 9.1;
> <https://gist.github.com/mikioh/6155250>
> ListenAfterKevent didn't work well.
OK, I see. So it wants to know whether it's passive or active socket
during association.
>> If it is (1), then I would suggest to call current all-in-one
>> runtime_pollInit after listen. Because after the split runtime_pollInit
>> becomes senseless operation, there is no need to call it before
>> runtime_pollOpen.
>
> Sounds reasonable to me. Alex, do you agree with Dmitry's proposal?
runtime_pollOpen must follow newFD on all code paths anyway. And
runtime_pollOpen must happen before any other async operations. If
that is not the case, then something is seriously wrong anyway.
OK, I know, I am just repeating myself.
Message from unknown
2013-08-05T14:16:20+00:00mikiourn:md5:11339dd3887e71607023609836c0a16f
Message from mikioh.mikioh@gmail.com
2013-08-05T14:19:31+00:00mikiourn:md5:6e846ff217520ab85f8108effbcad3a9
PTAL
Message from dvyukov@google.com
2013-08-05T14:40:47+00:00dvyukovurn:md5:f20aef6095e4d8a6cda1a8510300d32b
On 2013/08/05 14:19:31, mikio wrote:
> PTAL
Looks much better overall.
If fd.init() fails, you call fd.Close() which calls fd.pd.Evict()/Close(). They are not prepared for pd.runtimeCtx==0.
You can do something along the lines of:
func (pd *pollDesc) Close() {
if pd.runtimeCtx == 0 {
return
}
runtime_pollClose(pd.runtimeCtx)
pd.runtimeCtx = 0
}
Also please ensure that fd_poll_unix.go still works as well.
Message from unknown
2013-08-06T03:17:14+00:00mikiourn:md5:a3b64eae7352587a1809efdcf3216a1f
Message from mikioh.mikioh@gmail.com
2013-08-06T03:29:06+00:00mikiourn:md5:56e0358f90939625d131e2ea6646af68
PTAL
Thanks. All addressed and changed the CL description.
> Also please ensure that fd_poll_unix.go still works as well.
FreeBSD is fine.
Looks like other bros. have unrelated troubles but doesn't matter.
Digressions, it seems like we need a bit more file hand-off tests
for FileConn, FIleListener and FilePacketConn.
Message from unknown
2013-08-06T06:27:06+00:00mikiourn:md5:8d43c3038cbad4e4a0618053e15be4ef
Message from mikioh.mikioh@gmail.com
2013-08-06T06:32:57+00:00mikiourn:md5:71a31b8d3bbb42c1145a91cb4c6994e3
And Evict, sidekick of Close.
On 2013/08/06 03:29:06, mikio wrote:
> PTAL
>
> Thanks. All addressed and changed the CL description.
>
> > Also please ensure that fd_poll_unix.go still works as well.
>
> FreeBSD is fine.
> Looks like other bros. have unrelated troubles but doesn't matter.
> Digressions, it seems like we need a bit more file hand-off tests
> for FileConn, FIleListener and FilePacketConn.
Message from alex.brainman@gmail.com
2013-08-06T06:56:58+00:00brainmanurn:md5:242f8fd2353ccae96b9a7922f3eb6b73
Perhaps I am missing something here, but why (*netFD).init cannot be part of newFD?
Alex
Message from dvyukov@google.com
2013-08-06T09:03:42+00:00dvyukovurn:md5:caa3cedbc456829b29363cd65b2214b5
On 2013/08/06 06:56:58, brainman wrote:
> Perhaps I am missing something here, but why (*netFD).init cannot be part of
> newFD?
On BSD systems it's necessary to associate fd with kqueue only after listen call. This change allows such flexibility.
Message from dvyukov@google.com
2013-08-06T09:04:21+00:00dvyukovurn:md5:754b99ffce4a5937b95f679d634801fb
LGTM
Message from unknown
2013-08-06T14:42:20+00:00mikiourn:md5:2e4ea4ccd078bfab35c8cff40cef4a4f
Message from mikioh.mikioh@gmail.com
2013-08-06T14:42:41+00:00mikiourn:md5:a98879b73db6aa5152ef3b6040ea74f4
*** Submitted as https://code.google.com/p/go/source/detail?r=43e7b87397b6 ***
net: separate pollster initialization from network file descriptor allocation
Unlike the existing net package own pollster, runtime-integrated
network pollster on BSD variants, actually kqueue, requires a socket
that has beed passed to syscall.Listen previously for a stream
listener.
This CL separates pollDesc.Init (actually runtime_pollOpen) from newFD
to allow control of each state of sockets and adds init method to netFD
instead. Upcoming CLs will rearrange the call order of runtime-integrated
pollster and syscall functions like the following;
- For dialers that open active connections, runtime_pollOpen will be
called in between syscall.Bind and syscall.Connect.
- For stream listeners that open passive stream connections,
runtime_pollOpen will be called just after syscall.Listen.
- For datagram listeners that open datagram connections,
runtime_pollOpen will be called just after syscall.Bind.
This is in preparation for runtime-integrated network pollster for BSD
variants.
Update issue 5199
R=dvyukov, alex.brainman, minux.ma
CC=golang-dev
https://codereview.appspot.com/8608044