Re: [Qemu-devel] [PATCH v2 2/2] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Alex Bligh]

On 27 Aug 2013, at 09:23, Stefan Hajnoczi wrote:

> 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.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> include/qemu/timer.h | 17 ++++++++++++++
> qemu-timer.c         | 66 +++++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 69 insertions(+), 14 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.
> + *

I'm in two minds as to whether there is a problem with the comment or the
code here. Despite git's best endeavours, this is a comment on 
qemu_clock_has_timers,
which is called in qemu_clock_warp in cpus.c. I'm still a little confused as
to what thread(s) that can be called from.

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

timer_mod and timer_mod_ns are inevitably called having just read the
clock with qemu_clock_get. I am presuming these are thread safe (I haven't
checked) and if so the documentation for them should be similar adjusted.
Otherwise making these thread safe is all but useless.

Alex

> diff --git a/qemu-timer.c b/qemu-timer.c
> index ed3fcb2..04869f4 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;
> @@ -299,9 +322,11 @@ void timer_free(QEMUTimer *ts)
> /* stop a timer, but do not dealloc it */
> void timer_del(QEMUTimer *ts)
> {
> +    QEMUTimerList *timer_list = ts->timer_list;
>     QEMUTimer **pt, *t;
> 
> -    pt = &ts->timer_list->active_timers;
> +    qemu_mutex_lock(&timer_list->active_timers_lock);
> +    pt = &timer_list->active_timers;
>     for(;;) {
>         t = *pt;
>         if (!t)
> @@ -312,18 +337,21 @@ void timer_del(QEMUTimer *ts)
>         }
>         pt = &t->next;
>     }
> +    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);
> 
>     /* add the timer in the sorted list */
> -    pt = &ts->timer_list->active_timers;
> +    qemu_mutex_lock(&timer_list->active_timers_lock);
> +    pt = &timer_list->active_timers;
>     for(;;) {
>         t = *pt;
>         if (!timer_expired_ns(t, expire_time)) {
> @@ -334,12 +362,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 +379,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)
> @@ -376,13 +411,16 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
> 
>     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;
> +        qemu_mutex_unlock(&timer_list->active_timers_lock);
> 
>         /* run the callback (the timer list can be modified) */
>         ts->cb(ts->opaque);
> -- 
> 1.8.3.1
> 
> 
> 

-- 
Alex Bligh