Re: [Qemu-devel] [PATCH 11/19] use a bottom half to run timers

[Marcelo Tosatti]

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 <address@hidden>
> ---
>  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
> 
> 
>