| Message ID | 1377591839-2743-3-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <stefanha@redhat.com> > --- > 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 > > >
On Tue, Aug 27, 2013 at 09:55:22AM +0100, Alex Bligh wrote: > On 27 Aug 2013, at 09:23, Stefan Hajnoczi wrote: > > 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. Remember icount is a TCG feature and will only be accessed from a single thread. > > @@ -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. Ping Fan's series deals with thread-safe clock sources, this series only tackles the timer_list. We need to apply both series to be able to use timers from threads. Stefan
Il 27/08/2013 10:55, Alex Bligh ha scritto: >> + * 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. This really means that calling this function must be done with care because there is potential of races. As long as you use aio_notify properly so that qemu_clock_warp is called after every timer_mod, it should be fine. >> > @@ -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. rt_clock is thread safe. host_clock is thread safe but there's the question of where to run the notifiers. Ping Fan has made vm_clock thread safe for the non-icount case, probably I'll look at icount sometime. Paolo
Il 27/08/2013 10:23, Stefan Hajnoczi ha scritto: > 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 <stefanha@redhat.com> > --- > 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. > + * > * 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..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; I think deletion and modification of the list should happen without releasing the lock in the middle. Paolo > 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); >
On Thu, Aug 29, 2013 at 12:00:12PM +0200, Paolo Bonzini wrote: > Il 27/08/2013 10:23, Stefan Hajnoczi ha scritto: > > /* 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; > > I think deletion and modification of the list should happen without > releasing the lock in the middle. Thanks for explaining the race between two timer_mod_ns() calls to me on IRC. I have sent a new revision of this series with your fixes included. Stefan
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..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);
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 <stefanha@redhat.com> --- include/qemu/timer.h | 17 ++++++++++++++ qemu-timer.c | 66 +++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 69 insertions(+), 14 deletions(-)