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

[Stefan Weil]

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?

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. 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.

>>
>> 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.

Regards
Stefan