[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] qemu-timer: add QEMUClock->active_timers li
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] qemu-timer: add QEMUClock->active_timers list lock |
Date: |
Fri, 05 Jul 2013 15:01:41 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
Il 05/07/2013 14:39, Stefan Hajnoczi ha scritto:
> This patch makes QEMUClock->active_timers list iteration thread-safe. With
> additional patches, this will allow threads to use timers without holding the
> QEMU global mutex.
>
> The qemu_next_alarm_deadline() function was restructured to reduce code
> duplication, which would have gotten worse due to the extra locking
> calls. The new qemu_next_clock_deadline() function does the work of
> updating the nearest deadline.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
> qemu-timer.c | 96
> +++++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 69 insertions(+), 27 deletions(-)
>
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 4740da9..c773af0 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -29,6 +29,7 @@
> #include "hw/hw.h"
>
> #include "qemu/timer.h"
> +#include "qemu/thread.h"
> #ifdef CONFIG_POSIX
> #include <pthread.h>
> #endif
> @@ -46,6 +47,7 @@
>
> struct QEMUClock {
> QEMUTimer *active_timers;
> + QemuMutex active_timers_lock;
>
> NotifierList reset_notifiers;
> int64_t last;
> @@ -85,31 +87,38 @@ static bool qemu_timer_expired_ns(QEMUTimer *timer_head,
> int64_t current_time)
> return timer_head && (timer_head->expire_time <= current_time);
> }
>
> -static int64_t qemu_next_alarm_deadline(void)
> +static int64_t qemu_next_clock_deadline(QEMUClock *clock, int64_t delta)
> {
> - int64_t delta = INT64_MAX;
> - int64_t rtdelta;
> + int64_t expire_time, next;
> + bool has_timer = false;
>
> - if (!use_icount && vm_clock->enabled && vm_clock->active_timers) {
> - delta = vm_clock->active_timers->expire_time -
> - qemu_get_clock_ns(vm_clock);
> + if (!clock->enabled) {
> + return delta;
> }
> - if (host_clock->enabled && host_clock->active_timers) {
> - int64_t hdelta = host_clock->active_timers->expire_time -
> - qemu_get_clock_ns(host_clock);
> - if (hdelta < delta) {
> - delta = hdelta;
> - }
> +
> + qemu_mutex_lock(&clock->active_timers_lock);
> + if (clock->active_timers) {
> + has_timer = true;
> + expire_time = clock->active_timers->expire_time;
> }
Ideally, the main loop would only lock clocks that have an expired
timer. We can optimize it later, but perhaps you can add a FIXME comment.
> @@ -254,22 +264,41 @@ void qemu_clock_enable(QEMUClock *clock, bool enabled)
>
> int64_t qemu_clock_has_timers(QEMUClock *clock)
> {
> - return !!clock->active_timers;
> + bool has_timers;
> +
> + qemu_mutex_lock(&clock->active_timers_lock);
> + has_timers = !!clock->active_timers;
> + qemu_mutex_unlock(&clock->active_timers_lock);
> + return has_timers;
This doesn't need the lock; the result can change immediately after
qemu_clock_has_timers returns. On the other hand, this is likely a sign
that the resulting code is actually not thread safe.
I think you can remove the call to qemu_clock_has_timers(vm_clock) from
qemu_clock_warp. It will advance icount_warp_timer by INT32_MAX nsecs
(a couple of seconds), which is fine.
> }
>
> int64_t qemu_clock_expired(QEMUClock *clock)
> {
> - return (clock->active_timers &&
> - clock->active_timers->expire_time < qemu_get_clock_ns(clock));
> + bool has_timers;
> + int64_t expire_time;
> +
> + qemu_mutex_lock(&clock->active_timers_lock);
> + has_timers = clock->active_timers;
> + expire_time = clock->active_timers->expire_time;
> + qemu_mutex_unlock(&clock->active_timers_lock);
> +
> + return has_timers && expire_time < qemu_get_clock_ns(clock);
> }
>
> int64_t qemu_clock_deadline(QEMUClock *clock)
> {
> /* To avoid problems with overflow limit this to 2^32. */
> int64_t delta = INT32_MAX;
> + bool has_timers;
> + int64_t expire_time;
>
> - if (clock->active_timers) {
> - delta = clock->active_timers->expire_time - qemu_get_clock_ns(clock);
> + qemu_mutex_lock(&clock->active_timers_lock);
> + has_timers = clock->active_timers;
> + expire_time = clock->active_timers->expire_time;
> + qemu_mutex_unlock(&clock->active_timers_lock);
> +
> + if (has_timers) {
> + delta = expire_time - qemu_get_clock_ns(clock);
> }
> if (delta < 0) {
> delta = 0;
> @@ -298,8 +327,10 @@ void qemu_free_timer(QEMUTimer *ts)
> /* stop a timer, but do not dealloc it */
> void qemu_del_timer(QEMUTimer *ts)
> {
> + QEMUClock *clock = ts->clock;
> QEMUTimer **pt, *t;
>
> + qemu_mutex_lock(&clock->active_timers_lock);
> pt = &ts->clock->active_timers;
> for(;;) {
> t = *pt;
> @@ -311,18 +342,21 @@ void qemu_del_timer(QEMUTimer *ts)
> }
> pt = &t->next;
> }
> + qemu_mutex_unlock(&clock->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 qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
> {
> + QEMUClock *clock = ts->clock;
> QEMUTimer **pt, *t;
>
> qemu_del_timer(ts);
>
> /* add the timer in the sorted list */
> - pt = &ts->clock->active_timers;
> + qemu_mutex_lock(&clock->active_timers_lock);
> + pt = &clock->active_timers;
> for(;;) {
> t = *pt;
> if (!qemu_timer_expired_ns(t, expire_time)) {
> @@ -333,6 +367,7 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
> ts->expire_time = expire_time;
> ts->next = *pt;
> *pt = ts;
> + qemu_mutex_unlock(&clock->active_timers_lock);
>
> /* Rearm if necessary */
> if (pt == &ts->clock->active_timers) {
... qemu_clock_warp and qemu_rearm_alarm_timer can then be called
without the lock, from multiple threads.
For qemu_clock_warp, you can pass the deadline and call it under the lock.
For qemu_rearm_alarm_timer I think there are two choices. Either you
add a separate lock for the alarm timer, or you drop the alarm timer
concept completely and just rely on the poll() timeout.
> @@ -355,12 +390,16 @@ void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
> bool qemu_timer_pending(QEMUTimer *ts)
> {
> QEMUTimer *t;
> + QEMUClock *clock = ts->clock;
> +
> + qemu_mutex_lock(&clock->active_timers_lock);
> for (t = ts->clock->active_timers; t != NULL; t = t->next) {
> if (t == ts) {
> - return true;
> + break;
> }
> }
> - return false;
> + qemu_mutex_unlock(&clock->active_timers_lock);
> + return t;
> }
>
> bool qemu_timer_expired(QEMUTimer *timer_head, int64_t current_time)
> @@ -378,13 +417,16 @@ void qemu_run_timers(QEMUClock *clock)
>
> current_time = qemu_get_clock_ns(clock);
> for(;;) {
> + qemu_mutex_lock(&clock->active_timers_lock);
> ts = clock->active_timers;
> if (!qemu_timer_expired_ns(ts, current_time)) {
> + qemu_mutex_unlock(&clock->active_timers_lock);
> break;
> }
> /* remove timer from the list before calling the callback */
> clock->active_timers = ts->next;
> ts->next = NULL;
> + qemu_mutex_unlock(&clock->active_timers_lock);
When this pattern happens, I generally prefer to have a lock/unlock
outside the loop, and an unlock/lock around the callback. This makes
the invariants clearer IMO.
Paolo
> /* run the callback (the timer list can be modified) */
> ts->cb(ts->opaque);
>