From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52809) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEz1h-0006li-2s for qemu-devel@nongnu.org; Thu, 29 Aug 2013 06:00:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEz1Y-0003By-Jz for qemu-devel@nongnu.org; Thu, 29 Aug 2013 06:00:29 -0400 Received: from mail-ee0-x233.google.com ([2a00:1450:4013:c00::233]:41506) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEz1Y-0003Bn-8i for qemu-devel@nongnu.org; Thu, 29 Aug 2013 06:00:20 -0400 Received: by mail-ee0-f51.google.com with SMTP id c1so118267eek.10 for ; Thu, 29 Aug 2013 03:00:19 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <521F1BAC.902@redhat.com> Date: Thu, 29 Aug 2013 12:00:12 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1377591839-2743-1-git-send-email-stefanha@redhat.com> <1377591839-2743-3-git-send-email-stefanha@redhat.com> In-Reply-To: <1377591839-2743-3-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Ping Fan Liu , qemu-devel@nongnu.org, Alex Bligh 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 > --- > 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); >