Re: [Qemu-devel] [PATCH 4/4] timer: make qemu_clock_enable sync between disable and timer's cb

[Paolo Bonzini]

 On Aug 05 2013, Liu Ping Fan wrote:
> After disabling the QemuClock, we should make sure that no QemuTimers
> are still in flight. To implement that with light overhead, we resort
> to QemuEvent. The caller of disabling will wait on QemuEvent of each
> timerlist.
> 
> Note, qemu_clock_enable(foo,false) can _not_ be called from timer's cb.
> And the callers of qemu_clock_enable() should be sync by themselves,
> not protected by this patch.
> 
> Signed-off-by: Liu Ping Fan <address@hidden>
> ---
>  include/qemu/timer.h |  1 +
>  qemu-timer.c         | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 1363316..ca09ba2 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -85,6 +85,7 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
>  
>  int qemu_timeout_ns_to_ms(int64_t ns);
>  int qemu_poll_ns(GPollFD *fds, uint nfds, int64_t timeout);
> +/* The disable of clock can not be called in timer's cb */

See below for a more verbose version of the comment.  For
now leave it only in the .c file, we should add comments to
all of timer.h.

>  void qemu_clock_enable(QEMUClock *clock, bool enabled);
>  void qemu_clock_warp(QEMUClock *clock);
>  
> diff --git a/qemu-timer.c b/qemu-timer.c
> index ebe7597..5828107 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -71,6 +71,8 @@ struct QEMUTimerList {
>      QLIST_ENTRY(QEMUTimerList) list;
>      QEMUTimerListNotifyCB *notify_cb;
>      void *notify_opaque;
> +    /* light weight method to mark the end of timerlist's running */
> +    QemuEvent ev;
>  };
>  
>  struct QEMUTimer {
> @@ -92,6 +94,7 @@ static QEMUTimerList *timerlist_new_from_clock(QEMUClock 
> *clock)
>      QEMUTimerList *tl;
>  
>      tl = g_malloc0(sizeof(QEMUTimerList));
> +    qemu_event_init(&tl->ev, false);

The event should start as "set", since "set" means "not inside
qemu_run_timers".

>      tl->clock = clock;
>      QLIST_INSERT_HEAD(&clock->timerlists, tl, list);
>      return tl;
> @@ -145,12 +148,18 @@ void qemu_clock_notify(QEMUClock *clock)
>      }
>  }
>  
> +/* The disable of clock can _not_ be called from timer's cb */

/* Disabling the clock will wait for related timerlists to stop
 * executing qemu_run_timers.  Thus, this functions should not
 * be used from the callback of a timer that is based on @clock.
 * Doing so would cause a deadlock.
 */

>  void qemu_clock_enable(QEMUClock *clock, bool enabled)
>  {
> +    QEMUTimerList *tl;
>      bool old = clock->enabled;
>      clock->enabled = enabled;
>      if (enabled && !old) {
>          qemu_clock_notify(clock);
> +    } else if (!enabled && old) {
> +        QLIST_FOREACH(tl, &clock->timerlists, list) {
> +            qemu_event_wait(&tl->ev);
> +        }
>      }
>  }
>  
> @@ -419,6 +428,7 @@ bool timerlist_run_timers(QEMUTimerList *tl)
>      }
>  
>      current_time = qemu_get_clock_ns(tl->clock);
> +    qemu_event_reset(&tl->ev);

Race condition here.  You need to test clock->enabled while the
event is reset.  Otherwise you get:

 -------------------------------------------------------------------------
     thread 1 is running                thread 2 is running
     qemu_clock_enable(foo, false)      qemu_run_timers(tl);
 -------------------------------------------------------------------------
                 ** event is initially set **
                                          if (!clock->enabled) return;
        clock->enabled = false;
        qemu_event_wait(&tl->ev);
        return;
                                          qemu_event_reset(&tl->ev);
                                          invokes callback
                                          qemu_event_set(&tl->ev);
 -------------------------------------------------------------------------

violating the invariant that no callbacks are invoked after the return from
qemu_clock_enable(foo, false).

Paolo

>      for(;;) {
>          ts = tl->active_timers;
>          if (!qemu_timer_expired_ns(ts, current_time)) {
> @@ -432,6 +442,7 @@ bool timerlist_run_timers(QEMUTimerList *tl)
>          ts->cb(ts->opaque);
>          progress = true;
>      }
> +    qemu_event_set(&tl->ev);
>      return progress;
>  }
>  
> -- 
> 1.8.1.4
>