[Qemu-devel] [PATCH v1 1/1] io/channel-watch.c: Only select on what we are actually waiting for

[Qemu-devel] [PATCH v1 1/1] io/channel-watch.c: Only select on what we are actually waiting for

[Alistair Francis]

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(-)

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,
-- 
2.11.0

Re: [Qemu-devel] [PATCH v1 1/1] io/channel-watch.c: Only select on what we are actually waiting for

[Daniel P. Berrange]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v1 1/1] io/channel-watch.c: Only select on what we are actually waiting for

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v1 1/1] io/channel-watch.c: Only select on what we are actually waiting for

[Alistair Francis]

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