[Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Jan Kiszka]

No need for this indirection via qemu_notify_event. On Unix, we already
catch SIGALRM via signalfd (or its emulation) and run the handler
synchronously. Under Win32, handlers run in separate threads. So we just
need to grab the global lock around the handler execution.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

The Unix side looks safe to me, but I'm not yet 100% confident about
Win32. This is part of an ongoing effort to create separate alarm
timers over their own io-threads. A lengthy effort. But these bits
appear useful to me already.

 main-loop.c  |    2 --
 qemu-timer.c |   21 +++++++++++++--------
 qemu-timer.h |    1 -
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index eb3b6e6..1a6ea25 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -499,8 +499,6 @@ int main_loop_wait(int nonblocking)
     slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
 #endif
 
-    qemu_run_all_timers();
-
     /* Check bottom-halves last in case any of the earlier events triggered
        them.  */
     qemu_bh_poll();
diff --git a/qemu-timer.c b/qemu-timer.c
index 5aea94e..26411a2 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -441,7 +441,7 @@ uint64_t qemu_timer_expire_time_ns(QEMUTimer *ts)
     return qemu_timer_pending(ts) ? ts->expire_time : -1;
 }
 
-void qemu_run_all_timers(void)
+static void qemu_run_all_timers(void)
 {
     alarm_timer->pending = false;
 
@@ -457,11 +457,7 @@ void qemu_run_all_timers(void)
     }
 }
 
-#ifdef _WIN32
-static void CALLBACK host_alarm_handler(PVOID lpParam, BOOLEAN unused)
-#else
 static void host_alarm_handler(int host_signum)
-#endif
 {
     struct qemu_alarm_timer *t = alarm_timer;
     if (!t)
@@ -469,7 +465,7 @@ static void host_alarm_handler(int host_signum)
 
     t->expired = true;
     t->pending = true;
-    qemu_notify_event();
+    qemu_run_all_timers();
 }
 
 #if defined(__linux__)
@@ -613,9 +609,11 @@ static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
     if (!t) {
         return;
     }
+    qemu_mutex_lock_iothread();
     t->expired = true;
     t->pending = true;
-    qemu_notify_event();
+    qemu_run_all_timers();
+    qemu_mutex_unlock_iothread();
 }
 
 static int mm_start_timer(struct qemu_alarm_timer *t)
@@ -668,6 +666,13 @@ static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
     }
 }
 
+static void CALLBACK win32_alarm_handler(PVOID lpParam, BOOLEAN unused)
+{
+    qemu_mutex_lock_iothread();
+    host_alarm_handler(0);
+    qemu_mutex_unlock_iothread();
+}
+
 static int win32_start_timer(struct qemu_alarm_timer *t)
 {
     HANDLE hTimer;
@@ -679,7 +684,7 @@ static int win32_start_timer(struct qemu_alarm_timer *t)
        interval in the dynticks case.  */
     success = CreateTimerQueueTimer(&hTimer,
                           NULL,
-                          host_alarm_handler,
+                          win32_alarm_handler,
                           t,
                           1,
                           3600000,
diff --git a/qemu-timer.h b/qemu-timer.h
index f8af595..b9bf313 100644
--- a/qemu-timer.h
+++ b/qemu-timer.h
@@ -58,7 +58,6 @@ bool qemu_timer_expired(QEMUTimer *timer_head, int64_t current_time);
 uint64_t qemu_timer_expire_time_ns(QEMUTimer *ts);
 
 void qemu_run_timers(QEMUClock *clock);
-void qemu_run_all_timers(void);
 void configure_alarms(char const *opt);
 void init_clocks(void);
 int init_timer_alarm(void);
-- 
1.7.3.4

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Paolo Bonzini]

Il 23/08/2012 13:23, Jan Kiszka ha scritto:
> No need for this indirection via qemu_notify_event. On Unix, we already
> catch SIGALRM via signalfd (or its emulation) and run the handler
> synchronously. Under Win32, handlers run in separate threads. So we just
> need to grab the global lock around the handler execution.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> The Unix side looks safe to me, but I'm not yet 100% confident about
> Win32. This is part of an ongoing effort to create separate alarm
> timers over their own io-threads. A lengthy effort.

Can you expand on this?

The Win32 bits look fine, but it's a bit scary to make the Unix and
Win32 paths so different.  It works well until we have a BQL for timers,
but would this complicate shrinking the scope of the BQL?

Paolo

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Jan Kiszka]

On 2012-08-23 13:39, Paolo Bonzini wrote:
> Il 23/08/2012 13:23, Jan Kiszka ha scritto:
>> No need for this indirection via qemu_notify_event. On Unix, we already
>> catch SIGALRM via signalfd (or its emulation) and run the handler
>> synchronously. Under Win32, handlers run in separate threads. So we just
>> need to grab the global lock around the handler execution.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> The Unix side looks safe to me, but I'm not yet 100% confident about
>> Win32. This is part of an ongoing effort to create separate alarm
>> timers over their own io-threads. A lengthy effort.
> 
> Can you expand on this?

Well, this patch removes an indirection from timer event deliveries. So
it reduces overhead, though only noticeable if you have high-rate timers.

> 
> The Win32 bits look fine, but it's a bit scary to make the Unix and
> Win32 paths so different.  It works well until we have a BQL for timers,
> but would this complicate shrinking the scope of the BQL?

Nope, not yet. We continue to hold the BQL across qemu_run_all_timers.
Under Unix, this happens as qemu_iohandler_poll->sigfd_handler->
host_alarm_handler runs under BQL, under win32 due to explicit locking.
The plan is to only pull specifically marked alarm timers out of this
standard path, in the future.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Paolo Bonzini]

Il 23/08/2012 14:10, Jan Kiszka ha scritto:
>> Can you expand on this?
> 
> Well, this patch removes an indirection from timer event deliveries. So
> it reduces overhead, though only noticeable if you have high-rate timers.

Actually, timers (and bottom halves) are always run after iohandlers.
So the qemu_notify_event should already be completely useless for Unix,
even if we leave the host_alarm_handler indirection.

But this leaves out Windows, where your next task of (IIUC) having
multiple instances of struct qemu_alarm_timer would be complicated by
the qemu_notify_event.  I guess this is the original reason for your patch.

So, in order to remove the qemu_notify_event completely, what about not
using signals anymore for timers?  You could just tweak the select
timeout and drop all the -clock madness.  Zero syscalls, practically no
overhead.  If this is not precise enough, use timerfd on Linux only
(BTW, switching to an absolute deadline would be useful too).

>> The Win32 bits look fine, but it's a bit scary to make the Unix and
>> Win32 paths so different.  It works well until we have a BQL for timers,
>> but would this complicate shrinking the scope of the BQL?
> 
> Nope, not yet. We continue to hold the BQL across qemu_run_all_timers.
> Under Unix, this happens as qemu_iohandler_poll->sigfd_handler->
> host_alarm_handler runs under BQL, under win32 due to explicit locking.
> The plan is to only pull specifically marked alarm timers out of this
> standard path, in the future.

Ok, thanks for clarifying.

Paolo

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Jan Kiszka]

On 2012-08-23 14:24, Paolo Bonzini wrote:
> Il 23/08/2012 14:10, Jan Kiszka ha scritto:
>>> Can you expand on this?
>>
>> Well, this patch removes an indirection from timer event deliveries. So
>> it reduces overhead, though only noticeable if you have high-rate timers.
> 
> Actually, timers (and bottom halves) are always run after iohandlers.
> So the qemu_notify_event should already be completely useless for Unix,
> even if we leave the host_alarm_handler indirection.

Is there anything that requires this ordering for timers?

> 
> But this leaves out Windows, where your next task of (IIUC) having
> multiple instances of struct qemu_alarm_timer would be complicated by
> the qemu_notify_event.  I guess this is the original reason for your patch.

I'm not heading for multi-instance alarm timers or any kind of
optimization on Windows. It should just continue to work. Windows is
neither a high-performance nor a real-time platform for QEMU, IMHO.

> 
> So, in order to remove the qemu_notify_event completely, what about not
> using signals anymore for timers?  You could just tweak the select
> timeout and drop all the -clock madness.  Zero syscalls, practically no
> overhead.  If this is not precise enough, use timerfd on Linux only

Need to think about it. At least, real-time tasks will get proper
precision on Linux. Not sure if it will be sufficient on other hosts.

> (BTW, switching to an absolute deadline would be useful too).

Why? We aren't affected by clock adjustment with relative timeouts.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Paolo Bonzini]

Il 23/08/2012 15:01, Jan Kiszka ha scritto:
> On 2012-08-23 14:24, Paolo Bonzini wrote:
>> Il 23/08/2012 14:10, Jan Kiszka ha scritto:
>>>> Can you expand on this?
>>>
>>> Well, this patch removes an indirection from timer event deliveries. So
>>> it reduces overhead, though only noticeable if you have high-rate timers.
>>
>> Actually, timers (and bottom halves) are always run after iohandlers.
>> So the qemu_notify_event should already be completely useless for Unix,
>> even if we leave the host_alarm_handler indirection.
> 
> Is there anything that requires this ordering for timers?

No, I don't think so.  (And also not for bottom halves, since those
always do a qemu_notify_event).

>> But this leaves out Windows, where your next task of (IIUC) having
>> multiple instances of struct qemu_alarm_timer would be complicated by
>> the qemu_notify_event.  I guess this is the original reason for your patch.
> 
> I'm not heading for multi-instance alarm timers or any kind of
> optimization on Windows. It should just continue to work. Windows is
> neither a high-performance nor a real-time platform for QEMU, IMHO.

Yeah, making multi-instance alarm timers optional does it as well.  (As
long as it doesn't bit rot).

>> So, in order to remove the qemu_notify_event completely, what about not
>> using signals anymore for timers?  You could just tweak the select
>> timeout and drop all the -clock madness.  Zero syscalls, practically no
>> overhead.  If this is not precise enough, use timerfd on Linux only
> 
> Need to think about it. At least, real-time tasks will get proper
> precision on Linux. Not sure if it will be sufficient on other hosts.

Do we care (I put non-Linux POSIX just a little above Windows but not much)?

>> (BTW, switching to an absolute deadline would be useful too).
> 
> Why? We aren't affected by clock adjustment with relative timeouts.

Just the usual fact that the deadline is incorrect if QEMU is pre-empted
between computing the deadline and applying it.  It shouldn't really
matter for the iothread, because it's not CPU bound, but it's cleaner
since internally we store absolute deadlines anyway.

Paolo

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Jan Kiszka]

On 2012-08-23 15:11, Paolo Bonzini wrote:
>>> So, in order to remove the qemu_notify_event completely, what about not
>>> using signals anymore for timers?  You could just tweak the select
>>> timeout and drop all the -clock madness.  Zero syscalls, practically no
>>> overhead.  If this is not precise enough, use timerfd on Linux only
>>
>> Need to think about it. At least, real-time tasks will get proper
>> precision on Linux. Not sure if it will be sufficient on other hosts.
> 
> Do we care (I put non-Linux POSIX just a little above Windows but not much)?

Well, at least we should not regress.

For Windows I just found out that we support millisecond resolution in
os_host_main_loop_wait at best due to g_poll. That would be fine, but
what is g_poll really using? Likely not mm-timers...

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RFC][PATCH] qemu-timer: Run timers in alarm timer handler

[Paolo Bonzini]

Il 23/08/2012 20:09, Jan Kiszka ha scritto:
> On 2012-08-23 15:11, Paolo Bonzini wrote:
>>>> So, in order to remove the qemu_notify_event completely, what about not
>>>> using signals anymore for timers?  You could just tweak the select
>>>> timeout and drop all the -clock madness.  Zero syscalls, practically no
>>>> overhead.  If this is not precise enough, use timerfd on Linux only
>>>
>>> Need to think about it. At least, real-time tasks will get proper
>>> precision on Linux. Not sure if it will be sufficient on other hosts.
>>
>> Do we care (I put non-Linux POSIX just a little above Windows but not much)?
> 
> Well, at least we should not regress.
> 
> For Windows I just found out that we support millisecond resolution in
> os_host_main_loop_wait at best due to g_poll.

Right.  But mm timers and timer queues also use millisecond resolution.

> That would be fine, but
> what is g_poll really using? Likely not mm-timers...

It is using WaitForMultipleObjects, but what matters in Windows is not
really the timing source you're using.  All that matters is that you use
timeBeginPeriod to set the process quantum.  Windows will use the lowest
quantum requested by a running process.  So timeBeginPeriod will affect
the precision even if you use another time source than mm timers.

(In fact, timer queues do not work on Windows, but in my experience work
and are cheaper under Wine.  I wonder if it's because timer queues do
not call timeBeginPeriod...).

Paolo