diff mbox

[PULL,1/2] util/aio-win32: Only select on what we are actually waiting for

Message ID 20170717154026.32697-2-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi July 17, 2017, 3:40 p.m. UTC 
From: Alistair Francis <alistair.francis@xilinx.com>

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 9307b70e9876c4e9e3c4478524a32a23a3d5dd05.1499368180.git.alistair.francis@xilinx.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/aio-win32.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini July 17, 2017, 5:23 p.m. UTC | #1 
> +        if (io_read) {
> +            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
> +        }
> +
> +        if (io_write) {
> +            bitmask |= FD_WRITE | FD_CONNECT;
> +        }
> +
>          event = event_notifier_get_handle(&ctx->notifier);
> -        WSAEventSelect(node->pfd.fd, event,
> -                       FD_READ | FD_ACCEPT | FD_CLOSE |
> -                       FD_CONNECT | FD_WRITE | FD_OOB);
> +        WSAEventSelect(node->pfd.fd, event, bitmask);
>      }
>

As noticed by Eric, if the same socket is in use via both aio-win32 and
another GSource, or via multiple AioContexts, this would break because
there is only one WSAEventSelect mask for each socket handle.

It is probably not going to break anything in the case of aio-win32, but I
think it is worth at least a comment. And because WSAEventSelect is
edge-triggered (e.g. an FD_WRITE event doesn't trigger again until the next
write) it shouldn't be that bad for performance.  If there's time to revert
this patch, I think it would be preferable.

Paolo
Alistair Francis July 19, 2017, 8:05 a.m. UTC | #2 
On Mon, Jul 17, 2017 at 7:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> +        if (io_read) {
>> +            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
>> +        }
>> +
>> +        if (io_write) {
>> +            bitmask |= FD_WRITE | FD_CONNECT;
>> +        }
>> +
>>          event = event_notifier_get_handle(&ctx->notifier);
>> -        WSAEventSelect(node->pfd.fd, event,
>> -                       FD_READ | FD_ACCEPT | FD_CLOSE |
>> -                       FD_CONNECT | FD_WRITE | FD_OOB);
>> +        WSAEventSelect(node->pfd.fd, event, bitmask);
>>      }
>>
>
> As noticed by Eric, if the same socket is in use via both aio-win32 and
> another GSource, or via multiple AioContexts, this would break because
> there is only one WSAEventSelect mask for each socket handle.
>
> It is probably not going to break anything in the case of aio-win32, but I
> think it is worth at least a comment. And because WSAEventSelect is
> edge-triggered (e.g. an FD_WRITE event doesn't trigger again until the next
> write) it shouldn't be that bad for performance.  If there's time to revert
> this patch, I think it would be preferable.

It looks like this was applied, I'll be back in the office next week
should I try and revert this or just add a comment explaining the
limitation.

Either that or store the current mask and OR then all together.

Thanks,
Alistair

>
> Paolo
diff mbox

Patch

diff --git a/util/aio-win32.c b/util/aio-win32.c
index bca496a..d6d5e02 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -71,6 +71,7 @@  void aio_set_fd_handler(AioContext *ctx,
         }
     } else {
         HANDLE event;
+        long bitmask = 0;
 
         if (node == NULL) {
             /* Alloc and insert if it's not already there */
@@ -95,10 +96,16 @@  void aio_set_fd_handler(AioContext *ctx,
         node->io_write = io_write;
         node->is_external = is_external;
 
+        if (io_read) {
+            bitmask |= FD_READ | FD_ACCEPT | FD_CLOSE;
+        }
+
+        if (io_write) {
+            bitmask |= FD_WRITE | FD_CONNECT;
+        }
+
         event = event_notifier_get_handle(&ctx->notifier);
-        WSAEventSelect(node->pfd.fd, event,
-                       FD_READ | FD_ACCEPT | FD_CLOSE |
-                       FD_CONNECT | FD_WRITE | FD_OOB);
+        WSAEventSelect(node->pfd.fd, event, bitmask);
     }
 
     qemu_lockcnt_unlock(&ctx->list_lock);