Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Stanislav Vorobiov]

Hi everyone,

Any comments on this v3 patch ?

On 04/18/2014 08:24 PM, Stanislav Vorobiov wrote:
> From: Sangho Park <address@hidden>
> 
> g_poll has a problem on Windows when using
> timeouts < 10ms, in glib/gpoll.c:
> 
> /* If not, and we have a significant timeout, poll again with
>  * timeout then. Note that this will return indication for only
>  * one event, or only for messages. We ignore timeouts less than
>  * ten milliseconds as they are mostly pointless on Windows, the
>  * MsgWaitForMultipleObjectsEx() call will timeout right away
>  * anyway.
>  */
> if (retval == 0 && (timeout == INFINITE || timeout >= 10))
>   retval = poll_rest (poll_msgs, handles, nhandles, fds, nfds, timeout);
> 
> so whenever g_poll is called with timeout < 10ms it does
> a quick poll instead of wait, this causes significant performance
> degradation of QEMU, thus we should use WaitForMultipleObjectsEx
> directly
> 
> Signed-off-by: Stanislav Vorobiov <address@hidden>
> ---
>  include/glib-compat.h |   19 +++++++++
>  include/qemu-common.h |   12 ------
>  util/oslib-win32.c    |  112 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+), 12 deletions(-)
> 
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 8aa77af..1280fb2 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -24,4 +24,23 @@ static inline guint g_timeout_add_seconds(guint interval, 
> GSourceFunc function,
>  }
>  #endif
>  
> +#ifdef _WIN32
> +/*
> + * g_poll has a problem on Windows when using
> + * timeouts < 10ms, so use wrapper.
> + */
> +#define g_poll(fds, nfds, timeout) g_poll_fixed(fds, nfds, timeout)
> +gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
> +#elif !GLIB_CHECK_VERSION(2, 20, 0)
> +/*
> + * Glib before 2.20.0 doesn't implement g_poll, so wrap it to compile 
> properly
> + * on older systems.
> + */
> +static inline gint g_poll(GPollFD *fds, guint nfds, gint timeout)
> +{
> +    GMainContext *ctx = g_main_context_default();
> +    return g_main_context_get_poll_func(ctx)(fds, nfds, timeout);
> +}
> +#endif
> +
>  #endif
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index a998e8d..3f3fd60 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -124,18 +124,6 @@ int qemu_main(int argc, char **argv, char **envp);
>  void qemu_get_timedate(struct tm *tm, int offset);
>  int qemu_timedate_diff(struct tm *tm);
>  
> -#if !GLIB_CHECK_VERSION(2, 20, 0)
> -/*
> - * Glib before 2.20.0 doesn't implement g_poll, so wrap it to compile 
> properly
> - * on older systems.
> - */
> -static inline gint g_poll(GPollFD *fds, guint nfds, gint timeout)
> -{
> -    GMainContext *ctx = g_main_context_default();
> -    return g_main_context_get_poll_func(ctx)(fds, nfds, timeout);
> -}
> -#endif
> -
>  /**
>   * is_help_option:
>   * @s: string to test
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 93f7d35..fa1f451 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -238,3 +238,115 @@ char *qemu_get_exec_dir(void)
>  {
>      return g_strdup(exec_dir);
>  }
> +
> +/*
> + * g_poll has a problem on Windows when using
> + * timeouts < 10ms, in glib/gpoll.c:
> + *
> + * // If not, and we have a significant timeout, poll again with
> + * // timeout then. Note that this will return indication for only
> + * // one event, or only for messages. We ignore timeouts less than
> + * // ten milliseconds as they are mostly pointless on Windows, the
> + * // MsgWaitForMultipleObjectsEx() call will timeout right away
> + * // anyway.
> + *
> + * if (retval == 0 && (timeout == INFINITE || timeout >= 10))
> + *   retval = poll_rest (poll_msgs, handles, nhandles, fds, nfds, timeout);
> + *
> + * So whenever g_poll is called with timeout < 10ms it does
> + * a quick poll instead of wait, this causes significant performance
> + * degradation of QEMU, thus we should use WaitForMultipleObjectsEx
> + * directly
> + */
> +gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout)
> +{
> +    guint i;
> +    HANDLE handles[MAXIMUM_WAIT_OBJECTS];
> +    gint nhandles = 0;
> +    int num_completed = 0;
> +
> +    for (i = 0; i < nfds; i++) {
> +        gint j;
> +
> +        if (fds[i].fd <= 0) {
> +            continue;
> +        }
> +
> +        /* don't add same handle several times
> +         */
> +        for (j = 0; j < nhandles; j++) {
> +            if (handles[j] == (HANDLE)fds[i].fd) {
> +                break;
> +            }
> +        }
> +
> +        if (j == nhandles) {
> +            if (nhandles == MAXIMUM_WAIT_OBJECTS) {
> +                fprintf(stderr, "Too many handles to wait for!\n");
> +                break;
> +            } else {
> +                handles[nhandles++] = (HANDLE)fds[i].fd;
> +            }
> +        }
> +    }
> +
> +    for (i = 0; i < nfds; ++i) {
> +        fds[i].revents = 0;
> +    }
> +
> +    if (timeout == -1) {
> +        timeout = INFINITE;
> +    }
> +
> +    if (nhandles == 0) {
> +        if (timeout == INFINITE) {
> +            return -1;
> +        } else {
> +            SleepEx(timeout, TRUE);
> +            return 0;
> +        }
> +    }
> +
> +    while (1) {
> +        DWORD res;
> +        gint j;
> +
> +        res = WaitForMultipleObjectsEx(nhandles, handles, FALSE,
> +            timeout, TRUE);
> +
> +        if (res == WAIT_FAILED) {
> +            for (i = 0; i < nfds; ++i) {
> +                fds[i].revents = 0;
> +            }
> +
> +            return -1;
> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
> +                   ((int)res < WAIT_OBJECT_0) ||
> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
> +            break;
> +        }
> +
> +        for (i = 0; i < nfds; ++i) {
> +            if (handles[res - WAIT_OBJECT_0] == (HANDLE)fds[i].fd) {
> +                fds[i].revents = fds[i].events;
> +            }
> +        }
> +
> +        ++num_completed;
> +
> +        if (nhandles <= 1) {
> +            break;
> +        }
> +
> +        /* poll the rest of the handles
> +         */
> +        for (j = res - WAIT_OBJECT_0 + 1; j < nhandles; j++) {
> +            handles[j - 1] = handles[j];
> +        }
> +        --nhandles;
> +
> +        timeout = 0;
> +    }
> +
> +    return num_completed;
> +}
>