diff mbox

[v5] glib: fix g_poll early timeout on windows

Message ID 1399538830-4443-1-git-send-email-s.vorobiov@samsung.com
State New
Headers show

Commit Message

Stanislav Vorobiov May 8, 2014, 8:47 a.m. UTC 
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 |    9 +++-
 util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+), 1 deletion(-)

Comments

Stefan Hajnoczi May 9, 2014, 6:57 p.m. UTC | #1 
On Thu, May 08, 2014 at 12:47:10PM +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 |    9 +++-
>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 120 insertions(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan
Stefan Weil May 16, 2014, 5 a.m. UTC | #2 
Am 09.05.2014 20:57, schrieb Stefan Hajnoczi:
> On Thu, May 08, 2014 at 12:47:10PM +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 |    9 +++-
>>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 120 insertions(+), 1 deletion(-)
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block
> 
> Stefan
> 



Hi,

the patch breaks hard disk i/o: a 64 bit executable built with MinGW-w64
will no longer boot a ReactOS image (BIOS fails to detect a bootable
disk). Booting a Linux CDROM seems to work and is indeed much faster.

Regards
Stefan
Stanislav Vorobiov May 16, 2014, 6:44 a.m. UTC | #3 
Hi,

Could you please provide URL to download that image so that I
could reproduce this problem ? Also, qemu command line would be nice.

btw, I won't be able to look into this during next week, so some help
from someone else would be nice. I'll try to take a look later today
however...

On 05/16/2014 09:00 AM, Stefan Weil wrote:
> Am 09.05.2014 20:57, schrieb Stefan Hajnoczi:
>> On Thu, May 08, 2014 at 12:47:10PM +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 |    9 +++-
>>>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 120 insertions(+), 1 deletion(-)
>>
>> Thanks, applied to my block tree:
>> https://github.com/stefanha/qemu/commits/block
>>
>> Stefan
>>
> 
> 
> 
> Hi,
> 
> the patch breaks hard disk i/o: a 64 bit executable built with MinGW-w64
> will no longer boot a ReactOS image (BIOS fails to detect a bootable
> disk). Booting a Linux CDROM seems to work and is indeed much faster.
> 
> Regards
> Stefan
> 
>
Stefan Weil May 16, 2014, 7:14 a.m. UTC | #4 
Am 16.05.2014 08:44, schrieb Stanislav Vorobiov:
> Hi,
>
> Could you please provide URL to download that image so that I
> could reproduce this problem ? Also, qemu command line would be nice.
>
> btw, I won't be able to look into this during next week, so some help
> from someone else would be nice. I'll try to take a look later today
> however...
>

There is no need to download an image. Any image will do for this test.
The command line also does not matter. I just tried this:

i386-softmmu/qemu-system-i386 -L pc-bios vl.c

With the current qemu trunk and a 64 bit default build (MinGW-w64 native
or cross), the BIOS cannot read the harddisk. When I undefine g_poll in
qemu-timer.c, QEMU can read it and the BIOS will report that vl.c is not
a bootable disk (which is quite obvious).

Stefan
Paolo Bonzini May 16, 2014, 11:32 a.m. UTC | #5 
Il 16/05/2014 07:00, Stefan Weil ha scritto:
> Hi,
>
> the patch breaks hard disk i/o: a 64 bit executable built with MinGW-w64
> will no longer boot a ReactOS image (BIOS fails to detect a bootable
> disk). Booting a Linux CDROM seems to work and is indeed much faster.

What about a 32-bit executable?

Paolo
Stefan Weil May 16, 2014, 6:01 p.m. UTC | #6 
Am 16.05.2014 13:32, schrieb Paolo Bonzini:
> Il 16/05/2014 07:00, Stefan Weil ha scritto:
>> Hi,
>>
>> the patch breaks hard disk i/o: a 64 bit executable built with MinGW-w64
>> will no longer boot a ReactOS image (BIOS fails to detect a bootable
>> disk). Booting a Linux CDROM seems to work and is indeed much faster.
>
> What about a 32-bit executable?
>
> Paolo

They show the same effect. When I reported the problem, I had no test
result for 32 bit, now I have one. Native builds on Windows are terribly
slow and take a lot of time...

Summary: Neither 32 nor 64 bit qemu system emulation on Windows hosts
can read hard disk images with this patch applied. It's sufficient to
test any image and see whether the BIOS detects the hard disk.

Stefan
diff mbox

Patch

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 8d25900..1280fb2 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -24,7 +24,14 @@  static inline guint g_timeout_add_seconds(guint interval, GSourceFunc function,
 }
 #endif
 
-#if !GLIB_CHECK_VERSION(2, 20, 0)
+#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.
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 93f7d35..69552f7 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 < (int)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;
+}