Re: [Qemu-devel] [RFC] [PATCHv8 11/30] aio / timers: Add a notify callback to QEMUTimerList

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Aug 08, 2013 at 10:42:08PM +0100, Alex Bligh wrote:
> @@ -213,13 +214,41 @@ QEMUClock *timerlist_get_clock(QEMUTimerList *timer_list);
>  bool timerlist_run_timers(QEMUTimerList *timer_list);
>  
>  /**
> + * timerlist_set_notify_cb:
> + * @timer_list: the timer list to use
> + * @cb: the callback to call on notification
> + * @opaque: the opaque pointer to pass to the callback
> + *
> + * Set the notify callback for a timer list. The notifier
> + * callback is called when the clock is reenabled or a timer
> + * on the list is modified.
> + */
> +void timerlist_set_notify_cb(QEMUTimerList *timer_list,
> +                             QEMUTimerListNotifyCB *cb, void *opaque);

When looking at thread-safety I had to think about set_notify_cb() for a
while.  The issue is that we add the timerlist to the clock source's
->timerlists *before* notify_cb has been assigned.

This could be a problem is another thread re-enables the clock source
while we are still in timerlist_new().

In practice it is not an issue when AioContexts are created under the
global mutex (that's the case today).

Still, it would be a little safer to drop set_notify_cb() and instead
pass in cb/opaque in timerlist_new().

Here is a patch that does this (against the previous revision of this
patch):

>From 75096b8fcafbac598ec0a5eab7a10cfb2e571fdf Mon Sep 17 00:00:00 2001
From: Stefan Hajnoczi <stefanha@redhat.com>
Date: Wed, 7 Aug 2013 15:44:02 +0200
Subject: [PATCH] qemu-timer: set notify_cb in timerlist_new()

Eliminate the race condition between creating a QEMUTimerList and
setting its notify_cb field.  This is important for multi-threading
scenarios where a timerlist can be notified before timerlist_new() has
returned.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/timer.h | 22 ++++++++--------------
 qemu-timer.c         | 25 ++++++++++++-------------
 2 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 9989d0e..935c259 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -207,13 +207,20 @@ void qemu_clock_notify(QEMUClock *clock);
 /**
  * timerlist_new:
  * @type: the clock type to associate with the timerlist
+ * @cb: the callback to call on notification
+ * @opaque: the opaque pointer to pass to the callback
  *
  * Create a new timerlist associated with the clock of
  * type @type.
  *
+ * The notifier callback is called when the clock is reenabled or a timer on
+ * the list is modified.
+ *
  * Returns: a pointer to the QEMUTimerList created
  */
-QEMUTimerList *timerlist_new(QEMUClockType type);
+QEMUTimerList *timerlist_new(QEMUClockType type,
+                             QEMUTimerListNotifyCB *cb,
+                             void *opaque);
 
 /**
  * timerlist_free:
@@ -282,19 +289,6 @@ QEMUClock *timerlist_get_clock(QEMUTimerList *timer_list);
 bool timerlist_run_timers(QEMUTimerList *timer_list);
 
 /**
- * timerlist_set_notify_cb:
- * @timer_list: the timer list to use
- * @cb: the callback to call on notification
- * @opaque: the opaque pointer to pass to the callback
- *
- * Set the notify callback for a timer list. The notifier
- * callback is called when the clock is reenabled or a timer
- * on the list is modified.
- */
-void timerlist_set_notify_cb(QEMUTimerList *timer_list,
-                             QEMUTimerListNotifyCB *cb, void *opaque);
-
-/**
  * timerlist_notify:
  * @timer_list: the timer list to use
  *
diff --git a/qemu-timer.c b/qemu-timer.c
index a39c4d6..8cb4fe7 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -87,7 +87,9 @@ static bool qemu_timer_expired_ns(QEMUTimer *timer_head, int64_t current_time)
     return timer_head && (timer_head->expire_time <= current_time);
 }
 
-static QEMUTimerList *timerlist_new_from_clock(QEMUClock *clock)
+static QEMUTimerList *timerlist_new_from_clock(QEMUClock *clock,
+                                               QEMUTimerListNotifyCB *cb,
+                                               void *opaque)
 {
     QEMUTimerList *timer_list;
 
@@ -101,13 +103,17 @@ static QEMUTimerList *timerlist_new_from_clock(QEMUClock *clock)
 
     timer_list = g_malloc0(sizeof(QEMUTimerList));
     timer_list->clock = clock;
+    timer_list->notify_cb = cb;
+    timer_list->notify_opaque = opaque;
     QLIST_INSERT_HEAD(&clock->timerlists, timer_list, list);
     return timer_list;
 }
 
-QEMUTimerList *timerlist_new(QEMUClockType type)
+QEMUTimerList *timerlist_new(QEMUClockType type,
+                             QEMUTimerListNotifyCB *cb,
+                             void *opaque)
 {
-    return timerlist_new_from_clock(qemu_get_clock(type));
+    return timerlist_new_from_clock(qemu_get_clock(type), cb, opaque);
 }
 
 void timerlist_free(QEMUTimerList *timer_list)
@@ -131,7 +137,8 @@ QEMUClock *qemu_clock_new(QEMUClockType type)
     clock->enabled = true;
     clock->last = INT64_MIN;
     notifier_list_init(&clock->reset_notifiers);
-    clock->default_timerlist = timerlist_new_from_clock(clock);
+    QLIST_INIT(&clock->timerlists);
+    clock->default_timerlist = timerlist_new_from_clock(clock, NULL, NULL);
     return clock;
 }
 
@@ -239,13 +246,6 @@ QEMUTimerList *qemu_clock_get_default_timerlist(QEMUClock *clock)
     return clock->default_timerlist;
 }
 
-void timerlist_set_notify_cb(QEMUTimerList *timer_list,
-                             QEMUTimerListNotifyCB *cb, void *opaque)
-{
-    timer_list->notify_cb = cb;
-    timer_list->notify_opaque = opaque;
-}
-
 void timerlist_notify(QEMUTimerList *timer_list)
 {
     if (timer_list->notify_cb) {
@@ -433,8 +433,7 @@ void timerlistgroup_init(QEMUTimerListGroup tlg,
 {
     QEMUClockType type;
     for (type = 0; type < QEMU_CLOCK_MAX; type++) {
-        tlg[type] = timerlist_new(type);
-        timerlist_set_notify_cb(tlg[type], cb, opaque);
+        tlg[type] = timerlist_new(type, cb, opaque);
     }
 }
 
-- 
1.8.1.4