code review 8608044: net: separate pollster initialization from network file descriptor allocation

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

[bradfitz, 2013-06-05 21:04:42]

What's the status of this?

[mikio, 2013-06-05 21:36:34]

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

[iant, 2013-06-20 16:33:47]

R=dvyukov

[dvyukov, 2013-06-22 15:51:11]

Is it ready for review? It seems to be not mailed.

[dvyukov, 2013-06-24 11:18:15]

On 2013/06/22 15:51:11, dvyukov wrote:
> Is it ready for review? It seems to be not mailed.

Q=wait

[dvyukov, 2013-08-01 12:33:45]

Is this still necessary? I thought that bsd will reuse darwin code, which does
not need the change.

[mikio, 2013-08-01 13:05:41]

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?

[dvyukov, 2013-08-01 13:11:27]

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.

[mikio, 2013-08-01 13:22:44]

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...

[dvyukov, 2013-08-01 13:33:07]

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...

[mikio, 2013-08-01 23:50:15]

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.

[mikio, 2013-08-05 02:07:54]

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

[dvyukov, 2013-08-05 08:40:57]

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.

[mikio, 2013-08-05 11:47:16]

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?

[dvyukov, 2013-08-05 11:59:36]

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.

[mikio, 2013-08-05 14:19:31]

PTAL

[dvyukov, 2013-08-05 14:40:47]

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.

[mikio, 2013-08-06 03:29:06]

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.

[mikio, 2013-08-06 06:32:57]

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.

[brainman, 2013-08-06 06:56:58]

Perhaps I am missing something here, but why (*netFD).init cannot be part of
newFD?

Alex

[dvyukov, 2013-08-06 09:03:42]

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.

[dvyukov, 2013-08-06 09:04:21]

LGTM

[mikio, 2013-08-06 14:42:41]

*** 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