Re: [Qemu-devel] [PATCH 2/3] qemu-timer: add QEMUClock->active_timers list lock

[Paolo Bonzini <pbonzini@redhat.com>]

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 <stefanha@redhat.com>
> ---
>  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);
>