On Mon, Dec 21, 2009 at 09:09:22AM +0100, Paolo Bonzini wrote:
> Make the timer subsystem register its own bottom half instead of
> placing the bottom half code in the heart of the main loop. To
> test if an alarm timer is pending, just check if the bottom half is
> scheduled.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> vl.c | 68 ++++++++++++++++++++++++++++++++++++-----------------------------
> 1 files changed, 38 insertions(+), 30 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 78807f5..289aadc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -573,10 +573,17 @@ struct qemu_alarm_timer {
> void (*rearm)(struct qemu_alarm_timer *t);
> void *priv;
>
> + QEMUBH *bh;
> char expired;
> - char pending;
> };
>
> +static struct qemu_alarm_timer *alarm_timer;
> +
> +static inline int qemu_alarm_pending(void)
> +{
> + return qemu_bh_scheduled(alarm_timer->bh);
> +}
> +
> static inline int alarm_has_dynticks(struct qemu_alarm_timer *t)
> {
> return !!t->rearm;
> @@ -593,8 +600,6 @@ static void qemu_rearm_alarm_timer(struct
> qemu_alarm_timer *t)
> /* TODO: MIN_TIMER_REARM_US should be optimized */
> #define MIN_TIMER_REARM_US 250
>
> -static struct qemu_alarm_timer *alarm_timer;
> -
> #ifdef _WIN32
>
> struct qemu_alarm_win32 {
> @@ -874,7 +879,7 @@ void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
>
> /* Rearm if necessary */
> if (pt == &active_timers[ts->clock->type]) {
> - if (!alarm_timer->pending) {
> + if (!qemu_alarm_pending()) {
> qemu_rearm_alarm_timer(alarm_timer);
> }
> /* Interrupt execution to force deadline recalculation. */
> @@ -1001,6 +1006,31 @@ static const VMStateDescription vmstate_timers = {
>
> static void qemu_event_increment(void);
>
> +static void qemu_timer_bh(void *opaque)
> +{
> + struct qemu_alarm_timer *t = opaque;
> +
> + /* rearm timer, if not periodic */
> + if (t->expired) {
> + t->expired = 0;
> + qemu_rearm_alarm_timer(t);
> + }
> +
> + /* vm time timers */
> + if (vm_running) {
> + if (!cur_cpu || likely(!(cur_cpu->singlestep_enabled &
> SSTEP_NOTIMER)))
> + qemu_run_timers(&active_timers[QEMU_CLOCK_VIRTUAL],
> + qemu_get_clock(vm_clock));
> + }
> +
> + /* real time timers */
> + qemu_run_timers(&active_timers[QEMU_CLOCK_REALTIME],
> + qemu_get_clock(rt_clock));
> +
> + qemu_run_timers(&active_timers[QEMU_CLOCK_HOST],
> + qemu_get_clock(host_clock));
> +}
> +
> #ifdef _WIN32
> static void CALLBACK host_alarm_handler(UINT uTimerID, UINT uMsg,
> DWORD_PTR dwUser, DWORD_PTR dw1,
> @@ -1059,8 +1089,7 @@ static void host_alarm_handler(int host_signum)
> cpu_exit(next_cpu);
> }
> #endif
> - t->pending = 1;
> - qemu_notify_event();
> + qemu_bh_schedule(t->bh);
> }
> }
>
> @@ -1446,7 +1475,8 @@ static int init_timer_alarm(void)
> }
>
> /* first event is at time 0 */
> - t->pending = 1;
> + t->bh = qemu_bh_new(qemu_timer_bh, t);
> + qemu_bh_schedule(t->bh);
> alarm_timer = t;
You should probably make sure the bh handling is signal safe. Perhaps
use atomic test-and-set for bh->schedule on qemu_bh_poll, etc...
Also there's a similar problem with the expired flag
> + /* rearm timer, if not periodic */
> + if (t->expired) {
> + t->expired = 0;
> + qemu_rearm_alarm_timer(t);
> + }
(it was there before your patch).
BTW, if host_alarm_handler runs after is t->expired is cleared,
but before qemu_run_timers->callback->qemu_mod_timer, subsequent
qemu_mod_timer callbacks fail to rearm the alarm timer, in case
timer in question expires earlier?
Nice patchset!
> qemu_add_vm_change_state_handler(alarm_timer_on_change_state_rearm, t);
>
> @@ -3811,28 +3841,6 @@ void main_loop_wait(int timeout)
>
> slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
>
> - /* rearm timer, if not periodic */
> - if (alarm_timer->expired) {
> - alarm_timer->expired = 0;
> - qemu_rearm_alarm_timer(alarm_timer);
> - }
> -
> - alarm_timer->pending = 0;
> -
> - /* vm time timers */
> - if (vm_running) {
> - if (!cur_cpu || likely(!(cur_cpu->singlestep_enabled &
> SSTEP_NOTIMER)))
> - qemu_run_timers(&active_timers[QEMU_CLOCK_VIRTUAL],
> - qemu_get_clock(vm_clock));
> - }
> -
> - /* real time timers */
> - qemu_run_timers(&active_timers[QEMU_CLOCK_REALTIME],
> - qemu_get_clock(rt_clock));
> -
> - qemu_run_timers(&active_timers[QEMU_CLOCK_HOST],
> - qemu_get_clock(host_clock));
> -
> /* Check bottom-halves last in case any of the earlier events triggered
> them. */
> qemu_bh_poll();
> @@ -3888,7 +3896,7 @@ static void tcg_cpu_exec(void)
>
> if (!vm_running)
> break;
> - if (alarm_timer->pending)
> + if (qemu_alarm_pending())
> break;
> if (cpu_can_run(env))
> ret = qemu_cpu_exec(env);
> --
> 1.6.5.2
>
>
>
