Stefan Hajnoczi <stefa...@redhat.com> writes:
> Introduce QEMUTimerList->active_timers_lock to protect the linked list
> of active timers. This allows qemu_timer_mod_ns() to be called from any
> thread.
>
> Note that vm_clock is not thread-safe and its use of
> qemu_clock_has_timers() works fine today but is also not thread-safe.
>
> The purpose of this patch is to eventually let device models set or
> cancel timers from a vcpu thread without holding the global mutex.
I've applied this set to Paolo's rcu tree - I see a couple of routines
that appear to need the active_timers_lock:
(line 137 of qemu-timer.c in my tree)
void qemu_clock_notify(QEMUClockType type)
{
QEMUTimerList *timer_list;
QEMUClock *clock = qemu_clock_ptr(type);
QLIST_FOREACH(timer_list, &clock->timerlists, list) {
timerlist_notify(timer_list);
}
}
(line 228 of qemu-timer.c in my tree)
int64_t qemu_clock_deadline_ns_all(QEMUClockType type)
{
int64_t deadline = -1;
QEMUTimerList *timer_list;
QEMUClock *clock = qemu_clock_ptr(type);
QLIST_FOREACH(timer_list, &clock->timerlists, list) {
deadline = qemu_soonest_timeout(deadline,
timerlist_deadline_ns(timer_list));
}
return deadline;
}
I think these functions are always called now with the BQL held, so I
wonder if they are good candidates for RCU?
Mike
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> include/qemu/timer.h | 17 ++++++++++
> qemu-timer.c | 87
> ++++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 85 insertions(+), 19 deletions(-)
>
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index e4934dd..b58903b 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -115,6 +115,10 @@ static inline int64_t qemu_clock_get_us(QEMUClockType
> type)
> * Determines whether a clock's default timer list
> * has timers attached
> *
> + * Note that this function should not be used when other threads also access
> + * the timer list. The return value may be outdated by the time it is acted
> + * upon.
> + *
> * Returns: true if the clock's default timer list
> * has timers attached
> */
> @@ -271,6 +275,10 @@ void timerlist_free(QEMUTimerList *timer_list);
> *
> * Determine whether a timer list has active timers
> *
> + * Note that this function should not be used when other threads also access
> + * the timer list. The return value may be outdated by the time it is acted
> + * upon.
> + *
> * Returns: true if the timer list has timers.
> */
> bool timerlist_has_timers(QEMUTimerList *timer_list);
> @@ -512,6 +520,9 @@ void timer_free(QEMUTimer *ts);
> * @ts: the timer
> *
> * Delete a timer from the active list.
> + *
> + * This function is thread-safe but the timer and its timer list must not be
> + * freed while this function is running.
> */
> void timer_del(QEMUTimer *ts);
>
> @@ -521,6 +532,9 @@ void timer_del(QEMUTimer *ts);
> * @expire_time: the expiry time in nanoseconds
> *
> * Modify a timer to expire at @expire_time
> + *
> + * This function is thread-safe but the timer and its timer list must not be
> + * freed while this function is running.
> */
> void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
>
> @@ -531,6 +545,9 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
> *
> * Modify a timer to expiry at @expire_time, taking into
> * account the scale associated with the timer.
> + *
> + * This function is thread-safe but the timer and its timer list must not be
> + * freed while this function is running.
> */
> void timer_mod(QEMUTimer *ts, int64_t expire_timer);
>
> diff --git a/qemu-timer.c b/qemu-timer.c
> index ed3fcb2..e504747 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -66,6 +66,7 @@ QEMUClock qemu_clocks[QEMU_CLOCK_MAX];
>
> struct QEMUTimerList {
> QEMUClock *clock;
> + QemuMutex active_timers_lock;
> QEMUTimer *active_timers;
> QLIST_ENTRY(QEMUTimerList) list;
> QEMUTimerListNotifyCB *notify_cb;
> @@ -101,6 +102,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
> timer_list->clock = clock;
> timer_list->notify_cb = cb;
> timer_list->notify_opaque = opaque;
> + qemu_mutex_init(&timer_list->active_timers_lock);
> QLIST_INSERT_HEAD(&clock->timerlists, timer_list, list);
> return timer_list;
> }
> @@ -111,6 +113,7 @@ void timerlist_free(QEMUTimerList *timer_list)
> if (timer_list->clock) {
> QLIST_REMOVE(timer_list, list);
> }
> + qemu_mutex_destroy(&timer_list->active_timers_lock);
> g_free(timer_list);
> }
>
> @@ -163,9 +166,17 @@ bool qemu_clock_has_timers(QEMUClockType type)
>
> bool timerlist_expired(QEMUTimerList *timer_list)
> {
> - return (timer_list->active_timers &&
> - timer_list->active_timers->expire_time <
> - qemu_clock_get_ns(timer_list->clock->type));
> + int64_t expire_time;
> +
> + qemu_mutex_lock(&timer_list->active_timers_lock);
> + if (!timer_list->active_timers) {
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> + return false;
> + }
> + expire_time = timer_list->active_timers->expire_time;
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> +
> + return expire_time < qemu_clock_get_ns(timer_list->clock->type);
> }
>
> bool qemu_clock_expired(QEMUClockType type)
> @@ -182,13 +193,25 @@ bool qemu_clock_expired(QEMUClockType type)
> int64_t timerlist_deadline_ns(QEMUTimerList *timer_list)
> {
> int64_t delta;
> + int64_t expire_time;
>
> - if (!timer_list->clock->enabled || !timer_list->active_timers) {
> + if (!timer_list->clock->enabled) {
> return -1;
> }
>
> - delta = timer_list->active_timers->expire_time -
> - qemu_clock_get_ns(timer_list->clock->type);
> + /* The active timers list may be modified before the caller uses our
> return
> + * value but ->notify_cb() is called when the deadline changes.
> Therefore
> + * the caller should notice the change and there is no race condition.
> + */
> + qemu_mutex_lock(&timer_list->active_timers_lock);
> + if (!timer_list->active_timers) {
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> + return -1;
> + }
> + expire_time = timer_list->active_timers->expire_time;
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> +
> + delta = expire_time - qemu_clock_get_ns(timer_list->clock->type);
>
> if (delta <= 0) {
> return 0;
> @@ -296,12 +319,11 @@ void timer_free(QEMUTimer *ts)
> g_free(ts);
> }
>
> -/* stop a timer, but do not dealloc it */
> -void timer_del(QEMUTimer *ts)
> +static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
> {
> QEMUTimer **pt, *t;
>
> - pt = &ts->timer_list->active_timers;
> + pt = &timer_list->active_timers;
> for(;;) {
> t = *pt;
> if (!t)
> @@ -314,16 +336,28 @@ void timer_del(QEMUTimer *ts)
> }
> }
>
> +/* stop a timer, but do not dealloc it */
> +void timer_del(QEMUTimer *ts)
> +{
> + QEMUTimerList *timer_list = ts->timer_list;
> +
> + qemu_mutex_lock(&timer_list->active_timers_lock);
> + timer_del_locked(timer_list, ts);
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> +}
> +
> /* modify the current timer so that it will be fired when current_time
> >= expire_time. The corresponding callback will be called. */
> void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
> {
> + QEMUTimerList *timer_list = ts->timer_list;
> QEMUTimer **pt, *t;
>
> - timer_del(ts);
> + qemu_mutex_lock(&timer_list->active_timers_lock);
> + timer_del_locked(timer_list, ts);
>
> /* add the timer in the sorted list */
> - pt = &ts->timer_list->active_timers;
> + pt = &timer_list->active_timers;
> for(;;) {
> t = *pt;
> if (!timer_expired_ns(t, expire_time)) {
> @@ -334,12 +368,13 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
> ts->expire_time = expire_time;
> ts->next = *pt;
> *pt = ts;
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
>
> /* Rearm if necessary */
> - if (pt == &ts->timer_list->active_timers) {
> + if (pt == &timer_list->active_timers) {
> /* Interrupt execution to force deadline recalculation. */
> - qemu_clock_warp(ts->timer_list->clock->type);
> - timerlist_notify(ts->timer_list);
> + qemu_clock_warp(timer_list->clock->type);
> + timerlist_notify(timer_list);
> }
> }
>
> @@ -350,13 +385,19 @@ void timer_mod(QEMUTimer *ts, int64_t expire_time)
>
> bool timer_pending(QEMUTimer *ts)
> {
> + QEMUTimerList *timer_list = ts->timer_list;
> QEMUTimer *t;
> - for (t = ts->timer_list->active_timers; t != NULL; t = t->next) {
> + bool found = false;
> +
> + qemu_mutex_lock(&timer_list->active_timers_lock);
> + for (t = timer_list->active_timers; t != NULL; t = t->next) {
> if (t == ts) {
> - return true;
> + found = true;
> + break;
> }
> }
> - return false;
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> + return found;
> }
>
> bool timer_expired(QEMUTimer *timer_head, int64_t current_time)
> @@ -369,23 +410,31 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
> QEMUTimer *ts;
> int64_t current_time;
> bool progress = false;
> -
> + QEMUTimerCB *cb;
> + void *opaque;
> +
> if (!timer_list->clock->enabled) {
> return progress;
> }
>
> current_time = qemu_clock_get_ns(timer_list->clock->type);
> for(;;) {
> + qemu_mutex_lock(&timer_list->active_timers_lock);
> ts = timer_list->active_timers;
> if (!timer_expired_ns(ts, current_time)) {
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
> break;
> }
> +
> /* remove timer from the list before calling the callback */
> timer_list->active_timers = ts->next;
> ts->next = NULL;
> + cb = ts->cb;
> + opaque = ts->opaque;
> + qemu_mutex_unlock(&timer_list->active_timers_lock);
>
> /* run the callback (the timer list can be modified) */
> - ts->cb(ts->opaque);
> + cb(opaque);
> progress = true;
> }
> return progress;
> --
> 1.8.3.1
>
>
--
Mike Day | + 1 919 371-8786 | ncm...@ncultra.org
"Endurance is a Virtue"
