Patchwork [4/4] timer: make qemu_clock_enable sync between disable and timer's cb

login
register
mail settings
Submitter pingfank@linux.vnet.ibm.com
Date Aug. 5, 2013, 7:33 a.m.
Message ID <1375688006-16780-5-git-send-email-pingfank@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/264593/
State New
Headers show

Comments

pingfank@linux.vnet.ibm.com - Aug. 5, 2013, 7:33 a.m.
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 <pingfank@linux.vnet.ibm.com>
---
 include/qemu/timer.h |  1 +
 qemu-timer.c         | 11 +++++++++++
 2 files changed, 12 insertions(+)
Paolo Bonzini - Aug. 5, 2013, 10:53 a.m.
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 <pingfank@linux.vnet.ibm.com>
> ---
>  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
>

Patch

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 */
 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);
     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 */
 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);
     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;
 }