| Message ID | 468425bd8090e3a9fd7fc791db42e24d6c75315c.1499940552.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 13, 2017 at 03:15:49AM -0700, Alistair Francis wrote: > When calling WAEventSelect() only wait on events as specified by the > condition variable. This requires that the condition variable is set > correctly for the specific events that we need to wait for. Can you describe the actual problem / buggy behaviour you were seeing with the current code. > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > > io/channel-watch.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/io/channel-watch.c b/io/channel-watch.c > index 8640d1c464..d80722f496 100644 > --- a/io/channel-watch.c > +++ b/io/channel-watch.c > @@ -286,9 +286,21 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, > QIOChannelSocketSource *ssource; > > #ifdef WIN32 > - WSAEventSelect(socket, ioc->event, > - FD_READ | FD_ACCEPT | FD_CLOSE | > - FD_CONNECT | FD_WRITE | FD_OOB); > + long bitmask = 0; > + > + if (condition & (G_IO_IN | G_IO_PRI)) { > + bitmask |= FD_READ | FD_ACCEPT; > + } > + > + if (condition & G_IO_HUP) { > + bitmask |= FD_CLOSE; > + } > + > + if (condition & G_IO_OUT) { > + bitmask |= FD_WRITE | FD_CONNECT; > + } > + > + WSAEventSelect(socket, ioc->event, bitmask); > #endif I think the problem with doing this is that WSAEventSelect is a global setting that applies to the socket handle. If you use qio_channel_create_watch twice on the same socket with different events, the second watch will break the first watch by potentially discarding events that it requested. Regards, Daniel
On 13/07/2017 12:23, Daniel P. Berrange wrote: > On Thu, Jul 13, 2017 at 03:15:49AM -0700, Alistair Francis wrote: >> diff --git a/io/channel-watch.c b/io/channel-watch.c >> index 8640d1c464..d80722f496 100644 >> --- a/io/channel-watch.c >> +++ b/io/channel-watch.c >> @@ -286,9 +286,21 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, >> QIOChannelSocketSource *ssource; >> >> #ifdef WIN32 >> - WSAEventSelect(socket, ioc->event, >> - FD_READ | FD_ACCEPT | FD_CLOSE | >> - FD_CONNECT | FD_WRITE | FD_OOB); >> + long bitmask = 0; >> + >> + if (condition & (G_IO_IN | G_IO_PRI)) { >> + bitmask |= FD_READ | FD_ACCEPT; >> + } >> + >> + if (condition & G_IO_HUP) { >> + bitmask |= FD_CLOSE; >> + } >> + >> + if (condition & G_IO_OUT) { >> + bitmask |= FD_WRITE | FD_CONNECT; >> + } >> + >> + WSAEventSelect(socket, ioc->event, bitmask); >> #endif > > I think the problem with doing this is that WSAEventSelect is a global > setting that applies to the socket handle. If you use qio_channel_create_watch > twice on the same socket with different events, the second watch will break > the first watch by potentially discarding events that it requested. This makes sense, and it means the corresponding aio-win32 patch is wrong too. WSAEventSelect events are edge-triggered, so they shouldn't be too bad. In particular, they won't cause a busy wait. Paolo
On Thu, Jul 13, 2017 at 1:09 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 13/07/2017 12:23, Daniel P. Berrange wrote: >> On Thu, Jul 13, 2017 at 03:15:49AM -0700, Alistair Francis wrote: >>> diff --git a/io/channel-watch.c b/io/channel-watch.c >>> index 8640d1c464..d80722f496 100644 >>> --- a/io/channel-watch.c >>> +++ b/io/channel-watch.c >>> @@ -286,9 +286,21 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, >>> QIOChannelSocketSource *ssource; >>> >>> #ifdef WIN32 >>> - WSAEventSelect(socket, ioc->event, >>> - FD_READ | FD_ACCEPT | FD_CLOSE | >>> - FD_CONNECT | FD_WRITE | FD_OOB); >>> + long bitmask = 0; >>> + >>> + if (condition & (G_IO_IN | G_IO_PRI)) { >>> + bitmask |= FD_READ | FD_ACCEPT; >>> + } >>> + >>> + if (condition & G_IO_HUP) { >>> + bitmask |= FD_CLOSE; >>> + } >>> + >>> + if (condition & G_IO_OUT) { >>> + bitmask |= FD_WRITE | FD_CONNECT; >>> + } >>> + >>> + WSAEventSelect(socket, ioc->event, bitmask); >>> #endif >> >> I think the problem with doing this is that WSAEventSelect is a global >> setting that applies to the socket handle. If you use qio_channel_create_watch >> twice on the same socket with different events, the second watch will break >> the first watch by potentially discarding events that it requested. > > This makes sense, and it means the corresponding aio-win32 patch is > wrong too. Ah, I see that. > > WSAEventSelect events are edge-triggered, so they shouldn't be too bad. > In particular, they won't cause a busy wait. The problem I have seen is that threads get kicked at incorrect times for events that you aren't waiting for. It isn't a horrific issue, but it does contribute to extra resource usage. I don't see any nice way to get the value of lNetworkEvents back from Windows, so unless we keep track of it for a socket we can't just add to it. Is it even possible to assign two different events to a single socket? Every call seems to overwrite whatever was previously set. Are there cases where we call this two for different events on the same socket? Thanks, Alistair > > Paolo
diff --git a/io/channel-watch.c b/io/channel-watch.c index 8640d1c464..d80722f496 100644 --- a/io/channel-watch.c +++ b/io/channel-watch.c @@ -286,9 +286,21 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, QIOChannelSocketSource *ssource; #ifdef WIN32 - WSAEventSelect(socket, ioc->event, - FD_READ | FD_ACCEPT | FD_CLOSE | - FD_CONNECT | FD_WRITE | FD_OOB); + long bitmask = 0; + + if (condition & (G_IO_IN | G_IO_PRI)) { + bitmask |= FD_READ | FD_ACCEPT; + } + + if (condition & G_IO_HUP) { + bitmask |= FD_CLOSE; + } + + if (condition & G_IO_OUT) { + bitmask |= FD_WRITE | FD_CONNECT; + } + + WSAEventSelect(socket, ioc->event, bitmask); #endif source = g_source_new(&qio_channel_socket_source_funcs,
When calling WAEventSelect() only wait on events as specified by the condition variable. This requires that the condition variable is set correctly for the specific events that we need to wait for. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- io/channel-watch.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)