From patchwork Tue Aug 27 08:23:59 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 270064 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id BCA012C00C8 for ; Tue, 27 Aug 2013 18:25:04 +1000 (EST) Received: from localhost ([::1]:54815 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEEaE-0006ne-Ub for incoming@patchwork.ozlabs.org; Tue, 27 Aug 2013 04:25:02 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEEZY-0006cz-6D for qemu-devel@nongnu.org; Tue, 27 Aug 2013 04:24:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEEZR-0004SC-HZ for qemu-devel@nongnu.org; Tue, 27 Aug 2013 04:24:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5445) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEEZR-0004S8-9A for qemu-devel@nongnu.org; Tue, 27 Aug 2013 04:24:13 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7R8O6gM022535 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 27 Aug 2013 04:24:06 -0400 Received: from localhost (ovpn-112-35.ams2.redhat.com [10.36.112.35]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r7R8O4Qw017531; Tue, 27 Aug 2013 04:24:05 -0400 From: Stefan Hajnoczi To: Date: Tue, 27 Aug 2013 10:23:59 +0200 Message-Id: <1377591839-2743-3-git-send-email-stefanha@redhat.com> In-Reply-To: <1377591839-2743-1-git-send-email-stefanha@redhat.com> References: <1377591839-2743-1-git-send-email-stefanha@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Paolo Bonzini , Ping Fan Liu , Stefan Hajnoczi , Alex Bligh Subject: [Qemu-devel] [PATCH v2 2/2] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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; 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);