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

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

[Stefan Hajnoczi]

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.

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         | 70 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 69 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] 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 v2 2/2] 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: Stefan Hajnoczi <stefanha@redhat.com>
---
 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);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/2] qemu-timer: drop outdated signal safety comments

[Alex Bligh]

On 27 Aug 2013, at 09:23, Stefan Hajnoczi wrote:

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

Signed-off-by: Alex Bligh <alex@alex.org.uk>

Ooops - meant to delete these myself.

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

-- 
Alex Bligh

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

[Alex Bligh]

On 27 Aug 2013, at 09:23, Stefan Hajnoczi wrote:

> 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 <stefanha@redhat.com>
> ---
> 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.
> + *

I'm in two minds as to whether there is a problem with the comment or the
code here. Despite git's best endeavours, this is a comment on qemu_clock_has_timers,
which is called in qemu_clock_warp in cpus.c. I'm still a little confused as
to what thread(s) that can be called from.

>  * 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);

timer_mod and timer_mod_ns are inevitably called having just read the
clock with qemu_clock_get. I am presuming these are thread safe (I haven't
checked) and if so the documentation for them should be similar adjusted.
Otherwise making these thread safe is all but useless.

Alex

> 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);
> -- 
> 1.8.3.1
> 
> 
> 

-- 
Alex Bligh

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

[Stefan Hajnoczi]

On Tue, Aug 27, 2013 at 09:55:22AM +0100, Alex Bligh wrote:
> On 27 Aug 2013, at 09:23, Stefan Hajnoczi wrote:
> > 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.
> > + *
> 
> I'm in two minds as to whether there is a problem with the comment or the
> code here. Despite git's best endeavours, this is a comment on qemu_clock_has_timers,
> which is called in qemu_clock_warp in cpus.c. I'm still a little confused as
> to what thread(s) that can be called from.

Remember icount is a TCG feature and will only be accessed from a single
thread.

> > @@ -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);
> 
> timer_mod and timer_mod_ns are inevitably called having just read the
> clock with qemu_clock_get. I am presuming these are thread safe (I haven't
> checked) and if so the documentation for them should be similar adjusted.
> Otherwise making these thread safe is all but useless.

Ping Fan's series deals with thread-safe clock sources, this series only
tackles the timer_list.

We need to apply both series to be able to use timers from threads.

Stefan

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

[Paolo Bonzini]

Il 27/08/2013 10:55, Alex Bligh ha scritto:
>> + * 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.
>> + *
> 
> I'm in two minds as to whether there is a problem with the comment or the
> code here. Despite git's best endeavours, this is a comment on qemu_clock_has_timers,
> which is called in qemu_clock_warp in cpus.c. I'm still a little confused as
> to what thread(s) that can be called from.

This really means that calling this function must be done with care
because there is potential of races.  As long as you use aio_notify
properly so that qemu_clock_warp is called after every timer_mod, it
should be fine.
>> > @@ -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);
> timer_mod and timer_mod_ns are inevitably called having just read the
> clock with qemu_clock_get. I am presuming these are thread safe (I haven't
> checked) and if so the documentation for them should be similar adjusted.
> Otherwise making these thread safe is all but useless.

rt_clock is thread safe.  host_clock is thread safe but there's the
question of where to run the notifiers.  Ping Fan has made vm_clock
thread safe for the non-icount case, probably I'll look at icount sometime.

Paolo

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

[Paolo Bonzini]

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

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

[Stefan Hajnoczi]

On Thu, Aug 29, 2013 at 12:00:12PM +0200, Paolo Bonzini wrote:
> Il 27/08/2013 10:23, Stefan Hajnoczi ha scritto:
> >  /* 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.

Thanks for explaining the race between two timer_mod_ns() calls to me on
IRC.  I have sent a new revision of this series with your fixes included.

Stefan