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 <address@hidden> 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