[Qemu-devel] [PATCH] main-loop: Fix SetEvent() on uninitialized handle on win32

[Qemu-devel] [PATCH] main-loop: Fix SetEvent() on uninitialized handle on win32

[Michael Roth]

The __attribute__((constructor)) init_main_loop() automatically get
called if qemu-tool.o is linked in. On win32, this leads to
a qemu_notify_event() call which attempts to SetEvent() on a HANDLE that
won't be initialized until qemu_init_main_loop() is manually called,
breaking qemu-tools.o programs on Windows at runtime.

This patch checks for an initialized event handle before attempting to
set it, which is analoguous to how we deal with an unitialized
io_thread_fd in the posix implementation.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 main-loop.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index 692381c..62d95b9 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -164,7 +164,7 @@ static int qemu_signal_init(void)
 
 #else /* _WIN32 */
 
-HANDLE qemu_event_handle;
+HANDLE qemu_event_handle = NULL;
 
 static void dummy_event_handler(void *opaque)
 {
@@ -183,6 +183,9 @@ static int qemu_event_init(void)
 
 void qemu_notify_event(void)
 {
+    if (!qemu_event_handle) {
+        return;
+    }
     if (!SetEvent(qemu_event_handle)) {
         fprintf(stderr, "qemu_notify_event: SetEvent failed: %ld\n",
                 GetLastError());
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] main-loop: Fix SetEvent() on uninitialized handle on win32

[Paolo Bonzini]

On 01/21/2012 02:08 AM, Michael Roth wrote:
> The __attribute__((constructor)) init_main_loop() automatically get
> called if qemu-tool.o is linked in. On win32, this leads to
> a qemu_notify_event() call which attempts to SetEvent() on a HANDLE that
> won't be initialized until qemu_init_main_loop() is manually called,
> breaking qemu-tools.o programs on Windows at runtime.
>
> This patch checks for an initialized event handle before attempting to
> set it, which is analoguous to how we deal with an unitialized
> io_thread_fd in the posix implementation.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

[Qemu-devel] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Michael Roth]

In some cases initializing the alarm timers can lead to non-negligable
overhead from programs that link against qemu-tool.o. At least,
setting a max-resolution WinMM alarm timer via mm_start_timer() (the
current default for Windows) can increase the "tick rate" on Windows
OSs and affect frequency scaling, and in the case of tools that run
in guest OSs such has qemu-ga, the impact can be fairly dramatic
(+20%/20% user/sys time on a core 2 processor was observed from an idle
Windows XP guest).

This patch doesn't address the issue directly (not sure what a good
solution would be for Windows, or what other situations it might be
noticeable), but it at least limits the scope of the issue to programs
that "opt-in" to using the main-loop.c functions by only enabling alarm
timers when qemu_init_main_loop() is called, which is already required
to make use of those facilities, so existing users shouldn't be
affected.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 main-loop.c |    2 +-
 main-loop.h |   12 ++++++++++++
 qemu-tool.c |    3 ++-
 vl.c        |    5 +++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index 62d95b9..db23de0 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -199,7 +199,7 @@ static int qemu_signal_init(void)
 }
 #endif
 
-int qemu_init_main_loop(void)
+int main_loop_init(void)
 {
     int ret;
 
diff --git a/main-loop.h b/main-loop.h
index f971013..4987041 100644
--- a/main-loop.h
+++ b/main-loop.h
@@ -41,10 +41,22 @@
  * SIGUSR2, thread signals (SIGFPE, SIGILL, SIGSEGV, SIGBUS) and real-time
  * signals if available.  Remember that Windows in practice does not have
  * signals, though.
+ *
+ * In the case of QEMU tools, this will also start/initialize timers.
  */
 int qemu_init_main_loop(void);
 
 /**
+ * main_loop_init: Initializes main loop
+ *
+ * Internal (but shared for compatibility reasons) initialization routine
+ * for the main loop. This should not be used by applications directly,
+ * use qemu_init_main_loop() instead.
+ *
+ */
+int main_loop_init(void);
+
+/**
  * main_loop_wait: Run one iteration of the main loop.
  *
  * If @nonblocking is true, poll for events, otherwise suspend until
diff --git a/qemu-tool.c b/qemu-tool.c
index 6b69668..183a583 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -83,11 +83,12 @@ void qemu_clock_warp(QEMUClock *clock)
 {
 }
 
-static void __attribute__((constructor)) init_main_loop(void)
+int qemu_init_main_loop(void)
 {
     init_clocks();
     init_timer_alarm();
     qemu_clock_enable(vm_clock, false);
+    return main_loop_init();
 }
 
 void slirp_select_fill(int *pnfds, fd_set *readfds,
diff --git a/vl.c b/vl.c
index ba55b35..74a47e6 100644
--- a/vl.c
+++ b/vl.c
@@ -2136,6 +2136,11 @@ static void free_and_trace(gpointer mem)
     free(mem);
 }
 
+int qemu_init_main_loop(void)
+{
+    return main_loop_init();
+}
+
 int main(int argc, char **argv, char **envp)
 {
     const char *gdbstub_dev = NULL;
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Jamie Lokier]

Michael Roth wrote:
> In some cases initializing the alarm timers can lead to non-negligable
> overhead from programs that link against qemu-tool.o. At least,
> setting a max-resolution WinMM alarm timer via mm_start_timer() (the
> current default for Windows) can increase the "tick rate" on Windows
> OSs and affect frequency scaling, and in the case of tools that run
> in guest OSs such has qemu-ga, the impact can be fairly dramatic
> (+20%/20% user/sys time on a core 2 processor was observed from an idle
> Windows XP guest).
> 
> This patch doesn't address the issue directly (not sure what a good
> solution would be for Windows, or what other situations it might be
> noticeable),

Is this a timer that need to fire soon after setting, every time?

I wonder if a different kind of Windows timer, lower-resolution, could
be used if the timeout is longer.  If it has insufficient resolution,
it could be set to trigger a little early, then set a high-resolution
timer at that point.

Maybe that could help for Linux CONFIG_NOHZ guests?

-- Jamie

Re: [Qemu-devel] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Paolo Bonzini]

On 01/21/2012 09:39 PM, Jamie Lokier wrote:
> Is this a timer that need to fire soon after setting, every time?
>
> I wonder if a different kind of Windows timer, lower-resolution, could
> be used if the timeout is longer.  If it has insufficient resolution,
> it could be set to trigger a little early, then set a high-resolution
> timer at that point.
>
> Maybe that could help for Linux CONFIG_NOHZ guests?

No, it's an implementation detail of Windows multimedia timers.  Just 
enabling them apparently burns CPU.

There is another kind of timers for Windows but it didn't work reliably. 
  Finding out why would be the right fix, but anyway

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Michael Roth]

On 01/22/2012 06:32 AM, Paolo Bonzini wrote:
> On 01/21/2012 09:39 PM, Jamie Lokier wrote:
>> Is this a timer that need to fire soon after setting, every time?
>>
>> I wonder if a different kind of Windows timer, lower-resolution, could
>> be used if the timeout is longer. If it has insufficient resolution,
>> it could be set to trigger a little early, then set a high-resolution
>> timer at that point.
>>
>> Maybe that could help for Linux CONFIG_NOHZ guests?
>
> No, it's an implementation detail of Windows multimedia timers. Just
> enabling them apparently burns CPU.
>
> There is another kind of timers for Windows but it didn't work reliably.
> Finding out why would be the right fix, but anyway

I'd looked into timer queues, which the developer docs suggested had 
deprecated the use of mm timers, but I came across this which I figured 
was why mm was preferred (%30 average error for a 50ms periodic/oneshot) 
by the QEMU code:

http://www.virtualdub.org/blog/pivot/entry.php?id=272

Apparently Windows 7 has some fixes for drift that makes them reasonable 
for a clock source, but still fairly low-res for an event timer.

I suppose we could use some historical timer data to adaptively 
determine a reasonable minimum resolution to set...basically if the last 
timer fired X ns ago, we'd configure the resolution to be 1/10th of that 
periodic or whatever, and start them off low-res as James suggested to 
avoid unnecessary CPU burn, but it's a pretty loose heuristic.

>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Paolo
>
>

Re: [Qemu-devel] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Paolo Bonzini]

On 01/23/2012 01:12 AM, Michael Roth wrote:
> I'd looked into timer queues, which the developer docs suggested had
> deprecated the use of mm timers, but I came across this which I figured
> was why mm was preferred (%30 average error for a 50ms periodic/oneshot)
> by the QEMU code:
>
> http://www.virtualdub.org/blog/pivot/entry.php?id=272
>
> Apparently Windows 7 has some fixes for drift that makes them reasonable
> for a clock source, but still fairly low-res for an event timer.

On the other hand, timeBeginPeriod affect basically all time sources, 
not just MM timers.  Using timer queues together with timeBeginPeriod 
might work, after all.  However, it's not too interesting since 
timeBeginPeriod is what causes the additional CPU usage.

What is interesting is to move timeBeginPeriod/timeEndPeriod from 
qemu-timer.c elsewhere so that only QEMU uses it.  The tools do not need 
precise timekeeping anyway.

But while that would be good to have later when more tools use the 
shared main loop, your patch is still good for now.

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Stefan Hajnoczi]

On Sat, Jan 21, 2012 at 11:13:53AM -0600, Michael Roth wrote:
> In some cases initializing the alarm timers can lead to non-negligable
> overhead from programs that link against qemu-tool.o. At least,
> setting a max-resolution WinMM alarm timer via mm_start_timer() (the
> current default for Windows) can increase the "tick rate" on Windows
> OSs and affect frequency scaling, and in the case of tools that run
> in guest OSs such has qemu-ga, the impact can be fairly dramatic
> (+20%/20% user/sys time on a core 2 processor was observed from an idle
> Windows XP guest).
> 
> This patch doesn't address the issue directly (not sure what a good
> solution would be for Windows, or what other situations it might be
> noticeable), but it at least limits the scope of the issue to programs
> that "opt-in" to using the main-loop.c functions by only enabling alarm
> timers when qemu_init_main_loop() is called, which is already required
> to make use of those facilities, so existing users shouldn't be
> affected.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  main-loop.c |    2 +-
>  main-loop.h |   12 ++++++++++++
>  qemu-tool.c |    3 ++-
>  vl.c        |    5 +++++
>  4 files changed, 20 insertions(+), 2 deletions(-)

static struct qemu_alarm_timer alarm_timers[] = {
#ifndef _WIN32
#ifdef __linux__
    {"dynticks", dynticks_start_timer,
     dynticks_stop_timer, dynticks_rearm_timer},
#endif
    {"unix", unix_start_timer, unix_stop_timer, unix_rearm_timer},
#else
    {"mmtimer", mm_start_timer, mm_stop_timer, mm_rearm_timer},
    {"dynticks", win32_start_timer, win32_stop_timer, win32_rearm_timer},
#endif

It seems Windows host already has a "dynticks" implementation.  Have you
tried using this instead of "mmtimer"?

mm_start_timer() is using 1 ms intervals!

Stefan

Re: [Qemu-devel] [Qemu-trivial] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Paolo Bonzini]

On 01/27/2012 06:46 AM, Stefan Hajnoczi wrote:
> #ifndef _WIN32
> #ifdef __linux__
>      {"dynticks", dynticks_start_timer,
>       dynticks_stop_timer, dynticks_rearm_timer},
> #endif
>      {"unix", unix_start_timer, unix_stop_timer, unix_rearm_timer},
> #else
>      {"mmtimer", mm_start_timer, mm_stop_timer, mm_rearm_timer},
>      {"dynticks", win32_start_timer, win32_stop_timer, win32_rearm_timer},
> #endif
>
> It seems Windows host already has a "dynticks" implementation.  Have you
> tried using this instead of "mmtimer"?

The dynticks Win32 timer doesn't boot Linux successfully, even though 
under Wine it works and it is actually more reliable than mmtimer (which 
is why I haven't thrown it away yet).

> mm_start_timer() is using 1 ms intervals!

No, it's setting a 1 ms system quantum via timeBeginPeriod.  The actual 
implementation is using dynamic rather than periodic ticks (it has a 
rearm callback).  We threw away periodic timers a few months ago.

The problem is that Windows doesn't have something like Linux NOHZ and 
limits the timer resolution to the system quanta.  That's 1 ms for 
mmtimer and 10 ms (the default) for dynticks right now.  However, I 
suspect that the solution is to move timeBeginPeriod and timeEndPeriod 
from timer code to generic QEMU code, dynticks would start working on 
native Windows too.  Besides possibly fixing QEMU, it would definitely 
fix Michael's problem, too.  Tools do not need such a fine-grained 
timer, and shorter quanta cause the increased CPU usage that Michael 
observed.

However, Michael's patch makes sense as a cleanup anyway.  Since we have 
an initialization function, there's no need to have that _and_ a 
constructor.

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Stefan Hajnoczi]

On Sat, Jan 21, 2012 at 11:13:53AM -0600, Michael Roth wrote:
> In some cases initializing the alarm timers can lead to non-negligable
> overhead from programs that link against qemu-tool.o. At least,
> setting a max-resolution WinMM alarm timer via mm_start_timer() (the
> current default for Windows) can increase the "tick rate" on Windows
> OSs and affect frequency scaling, and in the case of tools that run
> in guest OSs such has qemu-ga, the impact can be fairly dramatic
> (+20%/20% user/sys time on a core 2 processor was observed from an idle
> Windows XP guest).
> 
> This patch doesn't address the issue directly (not sure what a good
> solution would be for Windows, or what other situations it might be
> noticeable), but it at least limits the scope of the issue to programs
> that "opt-in" to using the main-loop.c functions by only enabling alarm
> timers when qemu_init_main_loop() is called, which is already required
> to make use of those facilities, so existing users shouldn't be
> affected.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  main-loop.c |    2 +-
>  main-loop.h |   12 ++++++++++++
>  qemu-tool.c |    3 ++-
>  vl.c        |    5 +++++
>  4 files changed, 20 insertions(+), 2 deletions(-)

BTW the reason I suggest making Windows timers work efficiently
("dynticks") is because qemu-tool might legitimately want to make use of
QEMU timers.

Stefan

Re: [Qemu-devel] [PATCH] main-loop: For tools, initialize timers as part of qemu_init_main_loop()

[Anthony Liguori]

On 01/21/2012 11:13 AM, Michael Roth wrote:
> In some cases initializing the alarm timers can lead to non-negligable
> overhead from programs that link against qemu-tool.o. At least,
> setting a max-resolution WinMM alarm timer via mm_start_timer() (the
> current default for Windows) can increase the "tick rate" on Windows
> OSs and affect frequency scaling, and in the case of tools that run
> in guest OSs such has qemu-ga, the impact can be fairly dramatic
> (+20%/20% user/sys time on a core 2 processor was observed from an idle
> Windows XP guest).
>
> This patch doesn't address the issue directly (not sure what a good
> solution would be for Windows, or what other situations it might be
> noticeable), but it at least limits the scope of the issue to programs
> that "opt-in" to using the main-loop.c functions by only enabling alarm
> timers when qemu_init_main_loop() is called, which is already required
> to make use of those facilities, so existing users shouldn't be
> affected.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   main-loop.c |    2 +-
>   main-loop.h |   12 ++++++++++++
>   qemu-tool.c |    3 ++-
>   vl.c        |    5 +++++
>   4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/main-loop.c b/main-loop.c
> index 62d95b9..db23de0 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -199,7 +199,7 @@ static int qemu_signal_init(void)
>   }
>   #endif
>
> -int qemu_init_main_loop(void)
> +int main_loop_init(void)
>   {
>       int ret;
>
> diff --git a/main-loop.h b/main-loop.h
> index f971013..4987041 100644
> --- a/main-loop.h
> +++ b/main-loop.h
> @@ -41,10 +41,22 @@
>    * SIGUSR2, thread signals (SIGFPE, SIGILL, SIGSEGV, SIGBUS) and real-time
>    * signals if available.  Remember that Windows in practice does not have
>    * signals, though.
> + *
> + * In the case of QEMU tools, this will also start/initialize timers.
>    */
>   int qemu_init_main_loop(void);
>
>   /**
> + * main_loop_init: Initializes main loop
> + *
> + * Internal (but shared for compatibility reasons) initialization routine
> + * for the main loop. This should not be used by applications directly,
> + * use qemu_init_main_loop() instead.
> + *
> + */
> +int main_loop_init(void);
> +
> +/**
>    * main_loop_wait: Run one iteration of the main loop.
>    *
>    * If @nonblocking is true, poll for events, otherwise suspend until
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 6b69668..183a583 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -83,11 +83,12 @@ void qemu_clock_warp(QEMUClock *clock)
>   {
>   }
>
> -static void __attribute__((constructor)) init_main_loop(void)
> +int qemu_init_main_loop(void)
>   {
>       init_clocks();
>       init_timer_alarm();
>       qemu_clock_enable(vm_clock, false);
> +    return main_loop_init();
>   }
>
>   void slirp_select_fill(int *pnfds, fd_set *readfds,
> diff --git a/vl.c b/vl.c
> index ba55b35..74a47e6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2136,6 +2136,11 @@ static void free_and_trace(gpointer mem)
>       free(mem);
>   }
>
> +int qemu_init_main_loop(void)
> +{
> +    return main_loop_init();
> +}
> +
>   int main(int argc, char **argv, char **envp)
>   {
>       const char *gdbstub_dev = NULL;

Re: [Qemu-devel] [Qemu-trivial] [PATCH] main-loop: Fix SetEvent() on uninitialized handle on win32

[Stefan Hajnoczi]

On Fri, Jan 20, 2012 at 07:08:27PM -0600, Michael Roth wrote:
> diff --git a/main-loop.c b/main-loop.c
> index 692381c..62d95b9 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -164,7 +164,7 @@ static int qemu_signal_init(void)
>  
>  #else /* _WIN32 */
>  
> -HANDLE qemu_event_handle;
> +HANDLE qemu_event_handle = NULL;

Global variables are automatically zeroed, no need to assign NULL.

[Qemu-devel] [PATCH v2] main-loop: Fix SetEvent() on uninitialized handle on win32

[Michael Roth]

The __attribute__((constructor)) init_main_loop() automatically get
called if qemu-tool.o is linked in. On win32, this leads to
a qemu_notify_event() call which attempts to SetEvent() on a HANDLE that
won't be initialized until qemu_init_main_loop() is manually called,
breaking qemu-tools.o programs on Windows at runtime.

This patch checks for an initialized event handle before attempting to
set it, which is analoguous to how we deal with an unitialized
io_thread_fd in the posix implementation.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 main-loop.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index 692381c..db90ace 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -183,6 +183,9 @@ static int qemu_event_init(void)
 
 void qemu_notify_event(void)
 {
+    if (!qemu_event_handle) {
+        return;
+    }
     if (!SetEvent(qemu_event_handle)) {
         fprintf(stderr, "qemu_notify_event: SetEvent failed: %ld\n",
                 GetLastError());
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] main-loop: Fix SetEvent() on uninitialized handle on win32

[Anthony Liguori]

On 01/20/2012 07:08 PM, Michael Roth wrote:
> The __attribute__((constructor)) init_main_loop() automatically get
> called if qemu-tool.o is linked in. On win32, this leads to
> a qemu_notify_event() call which attempts to SetEvent() on a HANDLE that
> won't be initialized until qemu_init_main_loop() is manually called,
> breaking qemu-tools.o programs on Windows at runtime.
>
> This patch checks for an initialized event handle before attempting to
> set it, which is analoguous to how we deal with an unitialized
> io_thread_fd in the posix implementation.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   main-loop.c |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/main-loop.c b/main-loop.c
> index 692381c..62d95b9 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -164,7 +164,7 @@ static int qemu_signal_init(void)
>
>   #else /* _WIN32 */
>
> -HANDLE qemu_event_handle;
> +HANDLE qemu_event_handle = NULL;
>
>   static void dummy_event_handler(void *opaque)
>   {
> @@ -183,6 +183,9 @@ static int qemu_event_init(void)
>
>   void qemu_notify_event(void)
>   {
> +    if (!qemu_event_handle) {
> +        return;
> +    }
>       if (!SetEvent(qemu_event_handle)) {
>           fprintf(stderr, "qemu_notify_event: SetEvent failed: %ld\n",
>                   GetLastError());