[Qemu-devel] [PATCH v4 0/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Qemu-devel] [PATCH v4 0/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Stefan Hajnoczi]

v4:
 * Rebased & retested for easy review and merge
 * No code changes

v3:
 * Squashed Paolo's fixes and added his patch to avoid locking in timer_pending()

v2:
 * Rebased onto qemu.git/master following the merge of Alex's AioContext timers

The purpose of these patches is to eventually allow device models to set and
cancel timers without holding the global mutex.  When the device model runs in
a vcpu thread and the iothread processes timers, the
QEMUTimerList->active_timers must be protected from concurrent access.

Patch 1 is a clean-up.

Patch 2 is the entire change needed to protect ->active_timers.

Patch 3 makes timer_pending() run without a lock.

Paolo Bonzini (1):
  qemu-timer: do not take the lock in timer_pending

Stefan Hajnoczi (2):
  qemu-timer: drop outdated signal safety comments
  qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

 include/qemu/timer.h | 17 ++++++++++
 qemu-timer.c         | 92 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 81 insertions(+), 28 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/3] qemu-timer: drop outdated signal safety comments

[Stefan Hajnoczi]

host_alarm_handler() is invoked from the signal processing thread
(currently the iothread).  Previously we did processing in a real signal
handler with signalfd and therefore needed signal-safe timer code.

Today host_alarm_handler() just marks the alarm timer as expired/pending
and notifies the main loop using qemu_notify_event().

Therefore these outdated comments about signal safety can be dropped.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-timer.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 95ff47f..ed3fcb2 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -301,8 +301,6 @@ void timer_del(QEMUTimer *ts)
 {
     QEMUTimer **pt, *t;
 
-    /* NOTE: this code must be signal safe because
-       timer_expired() can be called from a signal. */
     pt = &ts->timer_list->active_timers;
     for(;;) {
         t = *pt;
@@ -325,8 +323,6 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
     timer_del(ts);
 
     /* add the timer in the sorted list */
-    /* NOTE: this code must be signal safe because
-       timer_expired() can be called from a signal. */
     pt = &ts->timer_list->active_timers;
     for(;;) {
         t = *pt;
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Stefan Hajnoczi]

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: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/timer.h | 17 ++++++++++
 qemu-timer.c         | 87 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 85 insertions(+), 19 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..e504747 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;
@@ -296,12 +319,11 @@ void timer_free(QEMUTimer *ts)
     g_free(ts);
 }
 
-/* stop a timer, but do not dealloc it */
-void timer_del(QEMUTimer *ts)
+static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
 {
     QEMUTimer **pt, *t;
 
-    pt = &ts->timer_list->active_timers;
+    pt = &timer_list->active_timers;
     for(;;) {
         t = *pt;
         if (!t)
@@ -314,16 +336,28 @@ void timer_del(QEMUTimer *ts)
     }
 }
 
+/* stop a timer, but do not dealloc it */
+void timer_del(QEMUTimer *ts)
+{
+    QEMUTimerList *timer_list = ts->timer_list;
+
+    qemu_mutex_lock(&timer_list->active_timers_lock);
+    timer_del_locked(timer_list, ts);
+    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);
+    qemu_mutex_lock(&timer_list->active_timers_lock);
+    timer_del_locked(timer_list, ts);
 
     /* add the timer in the sorted list */
-    pt = &ts->timer_list->active_timers;
+    pt = &timer_list->active_timers;
     for(;;) {
         t = *pt;
         if (!timer_expired_ns(t, expire_time)) {
@@ -334,12 +368,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 +385,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)
@@ -369,23 +410,31 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     QEMUTimer *ts;
     int64_t current_time;
     bool progress = false;
-   
+    QEMUTimerCB *cb;
+    void *opaque;
+
     if (!timer_list->clock->enabled) {
         return progress;
     }
 
     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;
+        cb = ts->cb;
+        opaque = ts->opaque;
+        qemu_mutex_unlock(&timer_list->active_timers_lock);
 
         /* run the callback (the timer list can be modified) */
-        ts->cb(ts->opaque);
+        cb(opaque);
         progress = true;
     }
     return progress;
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 3/3] qemu-timer: do not take the lock in timer_pending

[Stefan Hajnoczi]

From: Paolo Bonzini <pbonzini@redhat.com>

We can deduce the result from expire_time, by making it always -1 if
the timer is not in the active_timers list.  We need to check against
negative times passed to timer_mod_ns; clamping them to zero is not
a problem because the only clock that has a zero value at VM startup
is QEMU_CLOCK_VIRTUAL, and it is monotonic so it cannot be non-zero.
QEMU_CLOCK_HOST, instead, is not monotonic but it cannot go to negative
values unless the host time is seriously screwed up and points to
the 1960s.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-timer.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index e504747..6b62e88 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -312,6 +312,7 @@ void timer_init(QEMUTimer *ts,
     ts->cb = cb;
     ts->opaque = opaque;
     ts->scale = scale;
+    ts->expire_time = -1;
 }
 
 void timer_free(QEMUTimer *ts)
@@ -323,6 +324,7 @@ static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
 {
     QEMUTimer **pt, *t;
 
+    ts->expire_time = -1;
     pt = &timer_list->active_timers;
     for(;;) {
         t = *pt;
@@ -365,7 +367,7 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
         }
         pt = &t->next;
     }
-    ts->expire_time = expire_time;
+    ts->expire_time = MAX(expire_time, 0);
     ts->next = *pt;
     *pt = ts;
     qemu_mutex_unlock(&timer_list->active_timers_lock);
@@ -385,19 +387,7 @@ void timer_mod(QEMUTimer *ts, int64_t expire_time)
 
 bool timer_pending(QEMUTimer *ts)
 {
-    QEMUTimerList *timer_list = ts->timer_list;
-    QEMUTimer *t;
-    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) {
-            found = true;
-            break;
-        }
-    }
-    qemu_mutex_unlock(&timer_list->active_timers_lock);
-    return found;
+    return ts->expire_time >= 0;
 }
 
 bool timer_expired(QEMUTimer *timer_head, int64_t current_time)
@@ -429,6 +419,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         /* remove timer from the list before calling the callback */
         timer_list->active_timers = ts->next;
         ts->next = NULL;
+        ts->expire_time = -1;
         cb = ts->cb;
         opaque = ts->opaque;
         qemu_mutex_unlock(&timer_list->active_timers_lock);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 0/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Stefan Hajnoczi]

On Thu, Sep 12, 2013 at 11:02:17AM +0200, Stefan Hajnoczi wrote:
> v4:
>  * Rebased & retested for easy review and merge
>  * No code changes
> 
> v3:
>  * Squashed Paolo's fixes and added his patch to avoid locking in timer_pending()
> 
> v2:
>  * Rebased onto qemu.git/master following the merge of Alex's AioContext timers
> 
> The purpose of these patches is to eventually allow device models to set and
> cancel timers without holding the global mutex.  When the device model runs in
> a vcpu thread and the iothread processes timers, the
> QEMUTimerList->active_timers must be protected from concurrent access.
> 
> Patch 1 is a clean-up.
> 
> Patch 2 is the entire change needed to protect ->active_timers.
> 
> Patch 3 makes timer_pending() run without a lock.
> 
> Paolo Bonzini (1):
>   qemu-timer: do not take the lock in timer_pending
> 
> Stefan Hajnoczi (2):
>   qemu-timer: drop outdated signal safety comments
>   qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe
> 
>  include/qemu/timer.h | 17 ++++++++++
>  qemu-timer.c         | 92 ++++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 81 insertions(+), 28 deletions(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Mike Day]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> 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.

I've applied this set to Paolo's rcu tree - I see a couple of routines
that appear to need the active_timers_lock:

(line 137 of qemu-timer.c in my tree)
void qemu_clock_notify(QEMUClockType type)
{
    QEMUTimerList *timer_list;
    QEMUClock *clock = qemu_clock_ptr(type);
    QLIST_FOREACH(timer_list, &clock->timerlists, list) {
        timerlist_notify(timer_list);
    }
}

(line 228 of qemu-timer.c in my tree)
int64_t qemu_clock_deadline_ns_all(QEMUClockType type)
{
    int64_t deadline = -1;
    QEMUTimerList *timer_list;
    QEMUClock *clock = qemu_clock_ptr(type);
    QLIST_FOREACH(timer_list, &clock->timerlists, list) {
        deadline = qemu_soonest_timeout(deadline,
                                        timerlist_deadline_ns(timer_list));
    }
    return deadline;
}

I think these functions are always called now with the BQL held, so I
wonder if they are good candidates for RCU? 

Mike


> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/qemu/timer.h | 17 ++++++++++
>  qemu-timer.c         | 87 ++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 85 insertions(+), 19 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..e504747 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;
> @@ -296,12 +319,11 @@ void timer_free(QEMUTimer *ts)
>      g_free(ts);
>  }
>  
> -/* stop a timer, but do not dealloc it */
> -void timer_del(QEMUTimer *ts)
> +static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
>  {
>      QEMUTimer **pt, *t;
>  
> -    pt = &ts->timer_list->active_timers;
> +    pt = &timer_list->active_timers;
>      for(;;) {
>          t = *pt;
>          if (!t)
> @@ -314,16 +336,28 @@ void timer_del(QEMUTimer *ts)
>      }
>  }
>  
> +/* stop a timer, but do not dealloc it */
> +void timer_del(QEMUTimer *ts)
> +{
> +    QEMUTimerList *timer_list = ts->timer_list;
> +
> +    qemu_mutex_lock(&timer_list->active_timers_lock);
> +    timer_del_locked(timer_list, ts);
> +    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);
> +    qemu_mutex_lock(&timer_list->active_timers_lock);
> +    timer_del_locked(timer_list, ts);
>  
>      /* add the timer in the sorted list */
> -    pt = &ts->timer_list->active_timers;
> +    pt = &timer_list->active_timers;
>      for(;;) {
>          t = *pt;
>          if (!timer_expired_ns(t, expire_time)) {
> @@ -334,12 +368,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 +385,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)
> @@ -369,23 +410,31 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>      QEMUTimer *ts;
>      int64_t current_time;
>      bool progress = false;
> -   
> +    QEMUTimerCB *cb;
> +    void *opaque;
> +
>      if (!timer_list->clock->enabled) {
>          return progress;
>      }
>  
>      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;
> +        cb = ts->cb;
> +        opaque = ts->opaque;
> +        qemu_mutex_unlock(&timer_list->active_timers_lock);
>  
>          /* run the callback (the timer list can be modified) */
> -        ts->cb(ts->opaque);
> +        cb(opaque);
>          progress = true;
>      }
>      return progress;
> -- 
> 1.8.3.1
>
>

-- 

Mike Day | + 1 919 371-8786 | ncmike@ncultra.org
"Endurance is a Virtue"

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Alex Bligh]

On 30 Sep 2013, at 13:45, Mike Day wrote:

> I've applied this set to Paolo's rcu tree - I see a couple of routines
> that appear to need the active_timers_lock:
> 
> (line 137 of qemu-timer.c in my tree)
> void qemu_clock_notify(QEMUClockType type)
> {
>    QEMUTimerList *timer_list;
>    QEMUClock *clock = qemu_clock_ptr(type);
>    QLIST_FOREACH(timer_list, &clock->timerlists, list) {
>        timerlist_notify(timer_list);
>    }
> }
> 
> (line 228 of qemu-timer.c in my tree)
> int64_t qemu_clock_deadline_ns_all(QEMUClockType type)
> {
>    int64_t deadline = -1;
>    QEMUTimerList *timer_list;
>    QEMUClock *clock = qemu_clock_ptr(type);
>    QLIST_FOREACH(timer_list, &clock->timerlists, list) {
>        deadline = qemu_soonest_timeout(deadline,
>                                        timerlist_deadline_ns(timer_list));
>    }
>    return deadline;
> }
> 
> I think these functions are always called now with the BQL held, so I
> wonder if they are good candidates for RCU? 

These routines iterate through the list of timerlists held by
a clock.

They do not iterate through the list of active timers in a timer
list. I believe the latter is what active_timers_lock protects.

The list of timers attached to a clock is only modified when timers
are created and deleted which is (currently) under the BQL.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Mike Day]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

>
>
> On Mon, Sep 30, 2013 at 8:55 AM, Alex Bligh <alex@alex.org.uk> wrote:
> >
> >
> > On 30 Sep 2013, at 13:45, Mike Day wrote:
> >
> > > I've applied this set to Paolo's rcu tree - I see a couple of routines
> > > that appear to need the active_timers_lock:
> > >
> > > (line 137 of qemu-timer.c in my tree)
> > > void qemu_clock_notify(QEMUClockType type)
> > > {
> > >    QEMUTimerList *timer_list;
> > >    QEMUClock *clock = qemu_clock_ptr(type);
> > >    QLIST_FOREACH(timer_list, &clock->timerlists, list) {
> > >        timerlist_notify(timer_list);
> > >    }
> > > }
> > >
> > > (line 228 of qemu-timer.c in my tree)
> > > int64_t qemu_clock_deadline_ns_all(QEMUClockType type)
> > > {
> > >    int64_t deadline = -1;
> > >    QEMUTimerList *timer_list;
> > >    QEMUClock *clock = qemu_clock_ptr(type);
> > >    QLIST_FOREACH(timer_list, &clock->timerlists, list) {
> > >        deadline = qemu_soonest_timeout(deadline,
> > >
>  timerlist_deadline_ns(timer_list));
> > >    }
> > >    return deadline;
> > > }
> > >
> > > I think these functions are always called now with the BQL held, so I
> > > wonder if they are good candidates for RCU?
> >
> > These routines iterate through the list of timerlists held by
> > a clock.
> >
> > They do not iterate through the list of active timers in a timer
> > list. I believe the latter is what active_timers_lock protects.
> >
> > The list of timers attached to a clock is only modified when timers
> > are created and deleted which is (currently) under the BQL.
> >
>
>
Sorry, and thanks for the correction re: active_timers_lock. I should have
said that clock->timerlists may need its own mutex if and when we remove
the BQL from the timer code.

Mike

[-- Attachment #2: Type: text/html, Size: 2358 bytes --]

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Alex Bligh]

Mike,

>> > void qemu_clock_notify(QEMUClockType type)
...
>> > int64_t qemu_clock_deadline_ns_all(QEMUClockType type)
...
>> > I think these functions are always called now with the BQL held, so I
>> > wonder if they are good candidates for RCU?
>>
>> These routines iterate through the list of timerlists held by
>> a clock.
>>
>> They do not iterate through the list of active timers in a timer
>> list. I believe the latter is what active_timers_lock protects.
>>
>> The list of timers attached to a clock is only modified when timers
>> are created and deleted which is (currently) under the BQL.
>>
>
>
>
> Sorry, and thanks for the correction re: active_timers_lock. I should
> have said that clock->timerlists may need its own mutex if and when we
> remove the BQL from the timer code.

Correct. The list of timerlists is only modified when a
QEMUTimerListGroup is created or destroyed (currently when an
AioContext is created or destroyed), and that is done with the
BQL held.

As you point out, it's currently walked by a couple of functions
that also have the BQL held.

I think the most likely change here is that the walkers might
move outside the BQL. Given modification of this list is so rare,
the lock would be very very read heavy, so RCU is probably a
sensible option.

This link may (or may not) help in understanding:
  http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Mike Day]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

On Mon, Sep 30, 2013 at 9:34 AM, Alex Bligh <alex@alex.org.uk> wrote:
>>> > void qemu_clock_notify(QEMUClockType type)
>
> ...
>>>
>>> > int64_t qemu_clock_deadline_ns_all(QEMUClockType type)
>
> ...
>
>>> > I think these functions are always called now with the BQL held, so I
>>> > wonder if they are good candidates for RCU?
>>>
>>> These routines iterate through the list of timerlists held by
>>> a clock.
>>>
>>> They do not iterate through the list of active timers in a timer
>>> list. I believe the latter is what active_timers_lock protects.
>>>
>>> The list of timers attached to a clock is only modified when timers
>>> are created and deleted which is (currently) under the BQL.
>>
>> Sorry, and thanks for the correction re: active_timers_lock. I should
>> have said that clock->timerlists may need its own mutex if and when we
>> remove the BQL from the timer code.
>
>
> Correct. The list of timerlists is only modified when a
> QEMUTimerListGroup is created or destroyed (currently when an
> AioContext is created or destroyed), and that is done with the
> BQL held.
>
> As you point out, it's currently walked by a couple of functions
> that also have the BQL held.
>
> I think the most likely change here is that the walkers might
> move outside the BQL. Given modification of this list is so rare,
> the lock would be very very read heavy, so RCU is probably a
> sensible option.
>
> This link may (or may not) help in understanding:
>  http://blog.alex.org.uk/2013/08/24/changes-to-qemus-timer-system/

Alex, thanks for the link -

Mike

[-- Attachment #2: Type: text/html, Size: 2090 bytes --]

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Paolo Bonzini]

Il 30/09/2013 15:34, Alex Bligh ha scritto:
> 
> I think the most likely change here is that the walkers might
> move outside the BQL. Given modification of this list is so rare,
> the lock would be very very read heavy, so RCU is probably a
> sensible option.

I agree.  Keeping the write side on the BQL is sane, but RCU-protecting
the read side actually makes the rules simpler.

Mike, would you like to give it a shot?

Paolo

Re: [Qemu-devel] [PATCH v4 2/3] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Mike Day]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 30/09/2013 15:34, Alex Bligh ha scritto:
>> 
>> I think the most likely change here is that the walkers might
>> move outside the BQL. Given modification of this list is so rare,
>> the lock would be very very read heavy, so RCU is probably a
>> sensible option.
>
> I agree.  Keeping the write side on the BQL is sane, but RCU-protecting
> the read side actually makes the rules simpler.
>
> Mike, would you like to give it a shot?

Yes, I will. I'll have a patchset for review within a couple of days. 

Mike
-- 

Mike Day | + 1 919 371-8786 | ncmike@ncultra.org
"Endurance is a Virtue"