Re: [Qemu-devel] [PATCH] timer: fix qemu_poll_ns early timeout on windows

[Stanislav Vorobiov]

Hi,

Please see below

On 04/18/2014 10:46 AM, Stefan Weil wrote:
> Hi,
> 
> sorry, your patch was too late for QEMU 2.0. It remained unnoticed for
> two reasons:
> 
> * Patches for some special version should show this in the subject line:
>   [PATCH for 2.0] instead of [PATCH]
> 
> * CC'ing the maintainers helps a lot, as you see now :-)
> 
> More comments below.
> 
> 
> Am 18.04.2014 04:11, schrieb Sangho Park:
>> Hi, maintainers. 
>> Could you check this patch?
>> http://www.mail-archive.com/address@hidden/msg227161.html
>>
>> Thanks
>>
>> ps)
>> We've checked the http://wiki.qemu.org/Contribute/SubmitAPatch. Yet, if we
>> made any violation or mistake, let us know. We will appreciate your favor.
>>
>> -----Original Message-----
>> From: Stanislav Vorobiov [mailto:address@hidden 
>> Sent: Thursday, April 17, 2014 6:08 PM
>> To: address@hidden
>> Cc: address@hidden; address@hidden
>> Subject: Re: [Qemu-devel] [PATCH] timer: fix qemu_poll_ns early timeout on
>> windows
>>
>> Hi, everyone
>>
>> Any comments on this one ? This patch fixes pretty serious performance
>> issues on windows, it would be great to have this in 2.0.0
>>
>> On 04/15/2014 12:41 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
> 
> Can you quantify this performance degradation and specify your test
> scenario? Did you find the source of those small timeouts? Which timeout
> values are used there?
We don't have a separate test scenario, we've noticed that performance 
degradation
on tizen emulator after merging with QEMU 1.7. It turned out that it
was caused by this patch series:

e93379b039030c68d85693a4bee2b76f814108d2 aio / timers: Rename qemu_timer_* 
functions
...
91c68f143d6e707c5783b162292dce38ae358c19 aio / timers: remove 
dummy_io_handler_flush from tests/test-aio.c

The important part is main-loop.c:main_loop_wait which was:

    ret = os_host_main_loop_wait(timeout);

and became:

    if (timeout == UINT32_MAX) {
        timeout_ns = -1;
    } else {
        timeout_ns = (uint64_t)timeout * (int64_t)(SCALE_MS);
    }

    timeout_ns = qemu_soonest_timeout(timeout_ns,
                                      timerlistgroup_deadline_ns(
                                          &main_loop_tlg));

    ret = os_host_main_loop_wait(timeout_ns);

os_host_main_loop_wait then does:

    g_poll_ret = qemu_poll_ns(poll_fds, n_poll_fds + w->num, poll_timeout_ns);

So, the change made main_loop_wait take timer timeouts into account, thus,
qemu_poll_ns started to get called with values < 10ms and this caused problems 
on
windows since those waits simply turned into polls due to g_poll logic. This is
in fact g_poll's problem, timeout values passed to g_poll mean "at least", i.e.
g_poll MUST wait AT LEAST this time, it's allowed to wait more, but it's not 
allowed
to wait less. IMHO this should be reported to glib as a bug, but it'll be nice 
to fix
this in QEMU as well since it'll take too much time to get this into glib (if 
possible at all).

Do we need to make some separate test to demonstrate this performance 
degradation ?

> 
> Would it be sufficient to round any timeout > 0 and < 10 to 10 for
> Windows hosts? Maybe this could be done in qemu_timeout_ns_to_ms.
We tried that, it gets almost as good as with this patch, but this
makes timeouts like, say, 2ms wait for 10ms, so with this patch it's
still better.

> If
> this does not work, we still can use g_poll for timeout >= 10 and call a
> new Windows specific polling function for timeout < 10.
Does it have a point to separate things, if we've implemented this for timeouts 
< 10ms
why not use it for timeouts >= 10ms ? The code in this patch is in fact g_poll's
implementation without that "timeout >= 10" hack and it handles timeouts >= 10ms
as well as g_poll does.

> 
>>>
>>> Signed-off-by: Stanislav Vorobiov <address@hidden>
>>> ---
>>>  qemu-timer.c |   91
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 91 insertions(+)
>>>
>>> diff --git a/qemu-timer.c b/qemu-timer.c index e15ce47..9fb92cb 100644
>>> --- a/qemu-timer.c
>>> +++ b/qemu-timer.c
>>> @@ -315,6 +315,97 @@ int qemu_poll_ns(GPollFD *fds, guint nfds, int64_t
>> timeout)
>>>          ts.tv_nsec = timeout % 1000000000LL;
>>>          return ppoll((struct pollfd *)fds, nfds, &ts, NULL);
>>>      }
>>> +#elif defined(_WIN32)
>>> +    guint i;
>>> +    HANDLE handles[MAXIMUM_WAIT_OBJECTS];
>>> +    gint nhandles = 0;
>>> +    int num_completed = 0;
>>> +    gint timeout_ms = qemu_timeout_ns_to_ms(timeout);
>>> +
>>> +    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_ms == -1) {
>>> +        timeout_ms = INFINITE;
>>> +    }
>>> +
>>> +    if (nhandles == 0) {
>>> +        if (timeout_ms == INFINITE) {
>>> +            return -1;
>>> +        } else {
>>> +            SleepEx(timeout_ms, TRUE);
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +    while (1) {
>>> +        DWORD res;
>>> +        gint j;
>>> +
>>> +        res = WaitForMultipleObjectsEx(nhandles, handles, FALSE,
>>> +            timeout_ms, 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_ms = 0;
>>> +    }
>>> +
>>> +    return num_completed;
>>>  #else
>>>      return g_poll(fds, nfds, qemu_timeout_ns_to_ms(timeout));  #endif
>>>
>>
> 
> That code adds a new dependency on windows.h for qemu-timer.c. If we
> really need it, I'd prefer an implementation in util/oslib-win32.c.
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 ?

> 
> Regards
> Stefan
> 
>