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 <stefa...@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 <stefa...@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
