[v3] glib: fix g_poll early timeout on windows

From: Sangho Park <sangho1206.park@samsung.com>

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 <s.vorobiov@samsung.com>
---
 include/glib-compat.h |   19 +++++++++
 include/qemu-common.h |   12 ------
 util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 12 deletions(-)

Hi everyone,

Any comments on this v3 patch ?

On 04/18/2014 08:24 PM, Stanislav Vorobiov wrote:
> From: Sangho Park <sangho1206.park@samsung.com>
> 
> 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 <s.vorobiov@samsung.com>
> ---
>  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;
> +}
>

On 22 Apr 2014, at 10:03, Stanislav Vorobiov wrote:

>> + */
>> +#define g_poll(fds, nfds, timeout) g_poll_fixed(fds, nfds, timeout)
>> +gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);

I can't comment on the Windows specific bits of this patch (though
I have commented on what needs fixing), however, I'm confused by the
need for the above.

We call g_poll. With your patch (under Windows), we should call
something else.

Why not just move the whole of qemu_poll_ns to an OS specific
file (like aio-posix.c / aio-win32.c - even dropping it in those
files would be better), and just implement qemu_poll_ns so it
calls the right thing. IE don't even call gpoll under windows
(we don't under POSIX if ppoll is present), don't #define, etc.

Hi, see below

On 04/22/2014 01:21 PM, Alex Bligh wrote:
> 
> On 22 Apr 2014, at 10:03, Stanislav Vorobiov wrote:
> 
>>> + */
>>> +#define g_poll(fds, nfds, timeout) g_poll_fixed(fds, nfds, timeout)
>>> +gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
> 
> I can't comment on the Windows specific bits of this patch (though
> I have commented on what needs fixing), however, I'm confused by the
> need for the above.
> 
> We call g_poll. With your patch (under Windows), we should call
> something else.
> 
> Why not just move the whole of qemu_poll_ns to an OS specific
> file (like aio-posix.c / aio-win32.c - even dropping it in those
> files would be better), and just implement qemu_poll_ns so it
> calls the right thing. IE don't even call gpoll under windows
> (we don't under POSIX if ppoll is present), don't #define, etc.
> 
That's what I originally proposed:

>m.b. it makes sense to move entire qemu_poll_ns to oslib then ? like
>this patch to oslib-win32.c and the rest of the stuff to oslib-posix.c ?

But Stefan proposed to make it this
way (which is also reasonable, since we replace buggy g_poll with a fixed one)
should I change my patch and move all stuff to posix.c/win32.c ?

Il 22/04/2014 11:41, Stanislav Vorobiov ha scritto:
> >m.b. it makes sense to move entire qemu_poll_ns to oslib then ? like
> >this patch to oslib-win32.c and the rest of the stuff to oslib-posix.c ?
>
> But Stefan proposed to make it this
> way (which is also reasonable, since we replace buggy g_poll with a fixed one)
> should I change my patch and move all stuff to posix.c/win32.c ?

Both approaches are reasonable.  Let's start with this patch, everything 
else can be done on top.

Paolo

On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
> From: Sangho Park <sangho1206.park@samsung.com>
> 
> 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 <s.vorobiov@samsung.com>
> ---
>  include/glib-compat.h |   19 +++++++++
>  include/qemu-common.h |   12 ------
>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+), 12 deletions(-)

What is the status of this patch?

I haven't followed the discussions around the issue but can review/merge
if there is agreement now.

Stefan

Il 06/05/2014 14:23, Stefan Hajnoczi ha scritto:
>> > Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
>> > ---
>> >  include/glib-compat.h |   19 +++++++++
>> >  include/qemu-common.h |   12 ------
>> >  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 131 insertions(+), 12 deletions(-)
> What is the status of this patch?
>
> I haven't followed the discussions around the issue but can review/merge
> if there is agreement now.

The GNOME folks haven't followed up, so I think we should pick it.

Paolo

On 6 May 2014, at 13:38, Paolo Bonzini wrote:

> Il 06/05/2014 14:23, Stefan Hajnoczi ha scritto:
>>> > Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
>>> > ---
>>> >  include/glib-compat.h |   19 +++++++++
>>> >  include/qemu-common.h |   12 ------
>>> >  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> >  3 files changed, 131 insertions(+), 12 deletions(-)
>> What is the status of this patch?
>> 
>> I haven't followed the discussions around the issue but can review/merge
>> if there is agreement now.
> 
> The GNOME folks haven't followed up, so I think we should pick it.

I haven't checked it on win32 but was happy with it otherwise.

Am 06.05.2014 17:49, schrieb Alex Bligh:
> 
> On 6 May 2014, at 13:38, Paolo Bonzini wrote:
> 
>> Il 06/05/2014 14:23, Stefan Hajnoczi ha scritto:
>>>>> Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
>>>>> ---
>>>>>  include/glib-compat.h |   19 +++++++++
>>>>>  include/qemu-common.h |   12 ------
>>>>>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 131 insertions(+), 12 deletions(-)
>>> What is the status of this patch?
>>>
>>> I haven't followed the discussions around the issue but can review/merge
>>> if there is agreement now.
>>
>> The GNOME folks haven't followed up, so I think we should pick it.
> 
> I haven't checked it on win32 but was happy with it otherwise.


There was another patch on the list which moved g_poll code from
qemu-common.h to glib-compat.h, so when both patches get merged I expect
a (trivial) merge conflict.

The patch looks good. I already have applied it to my local queue, but I
won't be able to test it before end of next week. If someone wants to
pick it up earlier, I would not mind.

Cheers,
Stefan

On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:

Please fix the following compiler warning with gcc 4.8.2:

> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
> +                   ((int)res < WAIT_OBJECT_0) ||
> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
> +            break;
> +        }

util/oslib-win32.c: In function 'g_poll_fixed':
util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
                    ((int)res < WAIT_OBJECT_0) ||
		                         ^

Hi,

Hm, but (int)res expression is not unsigned, it's signed. I've also had this warning,
but with this expression: (res < WAIT_OBJECT_0), that's why I put (int) there. Could it be that
for some reason your compiler treats "int" and "unsigned int", that would be really strange though...

On 05/07/2014 12:02 PM, Stefan Hajnoczi wrote:
> On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
> 
> Please fix the following compiler warning with gcc 4.8.2:
> 
>> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
>> +                   ((int)res < WAIT_OBJECT_0) ||
>> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
>> +            break;
>> +        }
> 
> util/oslib-win32.c: In function 'g_poll_fixed':
> util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>                     ((int)res < WAIT_OBJECT_0) ||
> 		                         ^
>

On 7 May 2014, at 09:36, Stanislav Vorobiov wrote:

> Hi,
> 
> Hm, but (int)res expression is not unsigned, it's signed. I've also had this warning,
> but with this expression: (res < WAIT_OBJECT_0), that's why I put (int) there. Could it be that
> for some reason your compiler treats "int" and "unsigned int", that would be really strange though...

I suspect the problem is that WAIT_OBJECT_0 is defined as an unsigned long:

#define WAIT_OBJECT_0       ((STATUS_WAIT_0 ) + 0 )

#define STATUS_WAIT_0       ((DWORD)0x00000000L)

So IIRC under the 'usual conversions', and int compared with it will be cast to an unsigned long too.

I think you want to cast WAIT_OBJECT_0 to a long or similar.

Alex


> On 05/07/2014 12:02 PM, Stefan Hajnoczi wrote:
>> On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
>> 
>> Please fix the following compiler warning with gcc 4.8.2:
>> 
>>> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
>>> +                   ((int)res < WAIT_OBJECT_0) ||
>>> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
>>> +            break;
>>> +        }
>> 
>> util/oslib-win32.c: In function 'g_poll_fixed':
>> util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>                    ((int)res < WAIT_OBJECT_0) ||
>> 		                         ^
>> 
> 
> 
>

Hi,

Yes it's probably the cause, thanks.

On 05/07/2014 12:49 PM, Alex Bligh wrote:
> 
> On 7 May 2014, at 09:36, Stanislav Vorobiov wrote:
> 
>> Hi,
>>
>> Hm, but (int)res expression is not unsigned, it's signed. I've also had this warning,
>> but with this expression: (res < WAIT_OBJECT_0), that's why I put (int) there. Could it be that
>> for some reason your compiler treats "int" and "unsigned int", that would be really strange though...
> 
> I suspect the problem is that WAIT_OBJECT_0 is defined as an unsigned long:
> 
> #define WAIT_OBJECT_0       ((STATUS_WAIT_0 ) + 0 )
> 
> #define STATUS_WAIT_0       ((DWORD)0x00000000L)
> 
> So IIRC under the 'usual conversions', and int compared with it will be cast to an unsigned long too.
> 
> I think you want to cast WAIT_OBJECT_0 to a long or similar.
> 
> Alex
> 
> 
>> On 05/07/2014 12:02 PM, Stefan Hajnoczi wrote:
>>> On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
>>>
>>> Please fix the following compiler warning with gcc 4.8.2:
>>>
>>>> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
>>>> +                   ((int)res < WAIT_OBJECT_0) ||
>>>> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
>>>> +            break;
>>>> +        }
>>>
>>> util/oslib-win32.c: In function 'g_poll_fixed':
>>> util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>>                    ((int)res < WAIT_OBJECT_0) ||
>>> 		                         ^
>>>
>>
>>
>>
>