[Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Stefan Hajnoczi]

This series makes the following functions thread-safe:

  qemu_mod_timer_ns()
  qemu_mod_timer()
  qemu_del_timer()
  qemu_timer_pending()

The following were already thread-safe:

  qemu_free_timer()
  qemu_new_timer()
  qemu_timer_expired()

Now it is possible to use QEMUTimer outside the QEMU global mutex.  Timer
callbacks are still invoked from the main loop.  If a thread wishes to run
timer callbacks it must use a thread-safe QEMUBH (which Ping Fan Liu is working
on).

Note that host_clock is not thread-safe because it keeps state and invokes
reset notifiers.  Device emulation threads mostly care about vm_clock, so this
is not a problem.

Stefan Hajnoczi (3):
  qemu-timer: drop outdated signal safety comments
  qemu-timer: add QEMUClock->active_timers list lock
  qemu-timer: add qemu_alarm_timer->timer_modified_lock

 qemu-timer.c | 129 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 90 insertions(+), 39 deletions(-)

-- 
1.8.1.4

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

[Stefan Hajnoczi]

host_alarm_handler() may be invoked as a signal handler.  Previously we
did more processing in the signal handler 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 b2d95e2..4740da9 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -300,8 +300,6 @@ void qemu_del_timer(QEMUTimer *ts)
 {
     QEMUTimer **pt, *t;
 
-    /* NOTE: this code must be signal safe because
-       qemu_timer_expired() can be called from a signal. */
     pt = &ts->clock->active_timers;
     for(;;) {
         t = *pt;
@@ -324,8 +322,6 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
     qemu_del_timer(ts);
 
     /* add the timer in the sorted list */
-    /* NOTE: this code must be signal safe because
-       qemu_timer_expired() can be called from a signal. */
     pt = &ts->clock->active_timers;
     for(;;) {
         t = *pt;
-- 
1.8.1.4

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

[Stefan Hajnoczi]

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;
     }
-    if (rt_clock->enabled && rt_clock->active_timers) {
-        rtdelta = (rt_clock->active_timers->expire_time -
-                 qemu_get_clock_ns(rt_clock));
-        if (rtdelta < delta) {
-            delta = rtdelta;
-        }
+    qemu_mutex_unlock(&clock->active_timers_lock);
+    if (!has_timer) {
+        return delta;
     }
 
-    return delta;
+    next = expire_time - qemu_get_clock_ns(clock);
+    return MIN(next, delta);
+}
+
+static int64_t qemu_next_alarm_deadline(void)
+{
+    int64_t delta = INT64_MAX;
+
+    if (!use_icount) {
+        delta = qemu_next_clock_deadline(vm_clock, delta);
+    }
+    delta = qemu_next_clock_deadline(host_clock, delta);
+    return qemu_next_clock_deadline(rt_clock, delta);
 }
 
 static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
@@ -240,6 +249,7 @@ static QEMUClock *qemu_new_clock(int type)
     clock->enabled = true;
     clock->last = INT64_MIN;
     notifier_list_init(&clock->reset_notifiers);
+    qemu_mutex_init(&clock->active_timers_lock);
     return clock;
 }
 
@@ -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;
 }
 
 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) {
@@ -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);
 
         /* run the callback (the timer list can be modified) */
         ts->cb(ts->opaque);
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/3] qemu-timer: add qemu_alarm_timer->timer_modified_lock

[Stefan Hajnoczi]

When a timer's expiration time is changed we may need to rearm the alarm
timer.  Rearming is necessary when the nearest deadline changes.

This patch introduces the qemu_alarm_timer->timer_modified boolean and a
lock to protect it.  It moves rearming the alarm timer from inside
qemu_mod_timer_ns() to qemu_run_all_timers() since we cannot rearm
outside the QEMU global mutex.

The following code is dropped because we always kick the main loop now:

  /* Interrupt execution to force deadline recalculation.  */
  qemu_clock_warp(ts->clock);
  if (use_icount) {
      qemu_notify_event();
  }

cpus.c will invoke qemu_clock_warp() again since the main loop ran.

It is now safe to call qemu_mod_timer_ns() for outside the QEMU global
mutex.

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

diff --git a/qemu-timer.c b/qemu-timer.c
index c773af0..9500d12 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -78,6 +78,10 @@ struct qemu_alarm_timer {
 #endif
     bool expired;
     bool pending;
+
+    /* Was the nearest deadline timer modified (possibly by another thread)? */
+    QemuMutex timer_modified_lock;
+    bool timer_modified;
 };
 
 static struct qemu_alarm_timer *alarm_timer;
@@ -371,14 +375,10 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
 
     /* Rearm if necessary  */
     if (pt == &ts->clock->active_timers) {
-        if (!alarm_timer->pending) {
-            qemu_rearm_alarm_timer(alarm_timer);
-        }
-        /* Interrupt execution to force deadline recalculation.  */
-        qemu_clock_warp(ts->clock);
-        if (use_icount) {
-            qemu_notify_event();
-        }
+        qemu_mutex_lock(&alarm_timer->timer_modified_lock);
+        alarm_timer->timer_modified = true;
+        qemu_mutex_unlock(&alarm_timer->timer_modified_lock);
+        qemu_notify_event();
     }
 }
 
@@ -484,6 +484,8 @@ uint64_t qemu_timer_expire_time_ns(QEMUTimer *ts)
 
 void qemu_run_all_timers(void)
 {
+    bool timer_modified;
+
     alarm_timer->pending = false;
 
     /* vm time timers */
@@ -491,11 +493,20 @@ void qemu_run_all_timers(void)
     qemu_run_timers(rt_clock);
     qemu_run_timers(host_clock);
 
+    /* Check if qemu_mod_timer_ns() has been called */
+    qemu_mutex_lock(&alarm_timer->timer_modified_lock);
+    timer_modified = alarm_timer->timer_modified;
+    alarm_timer->timer_modified = false;
+    qemu_mutex_unlock(&alarm_timer->timer_modified_lock);
+
     /* rearm timer, if not periodic */
     if (alarm_timer->expired) {
         alarm_timer->expired = false;
         qemu_rearm_alarm_timer(alarm_timer);
+    } else if (timer_modified) {
+        qemu_rearm_alarm_timer(alarm_timer);
     }
+
 }
 
 #ifdef _WIN32
@@ -805,6 +816,8 @@ int init_timer_alarm(void)
         goto fail;
     }
 
+    qemu_mutex_init(&t->timer_modified_lock);
+
     atexit(quit_timers);
 #ifdef CONFIG_POSIX
     pthread_atfork(NULL, NULL, reinit_timers);
-- 
1.8.1.4

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

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Jan Kiszka]

On 2013-07-05 14:39, Stefan Hajnoczi wrote:
> This series makes the following functions thread-safe:
> 
>   qemu_mod_timer_ns()
>   qemu_mod_timer()
>   qemu_del_timer()
>   qemu_timer_pending()
> 
> The following were already thread-safe:
> 
>   qemu_free_timer()
>   qemu_new_timer()
>   qemu_timer_expired()
> 
> Now it is possible to use QEMUTimer outside the QEMU global mutex.  Timer
> callbacks are still invoked from the main loop.  If a thread wishes to run
> timer callbacks it must use a thread-safe QEMUBH (which Ping Fan Liu is working
> on).

What do you mean with this? We need main-loop independent timers for any
task that depends on timely alarm delivery. Do your patches keep this in
mind as well?

> 
> Note that host_clock is not thread-safe because it keeps state and invokes
> reset notifiers.  Device emulation threads mostly care about vm_clock, so this
> is not a problem.

I suppose you know that vm_clock cannot be read outside BQL yet due to
timers_state and, under TCG, icount. Any ideas regarding this already? I
didn't have to solve that problem so far as I only need CLOCK_REALTIME
outside BQL.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

On 2013-07-05 14:39, Stefan Hajnoczi wrote:
> host_alarm_handler() may be invoked as a signal handler.  Previously we
> did more processing in the signal handler and therefore needed
> signal-safe timer code.

Signal handlers run in the context of the signal processing thread, i.e.
the iothread so far.

> 
> 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 b2d95e2..4740da9 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -300,8 +300,6 @@ void qemu_del_timer(QEMUTimer *ts)
>  {
>      QEMUTimer **pt, *t;
>  
> -    /* NOTE: this code must be signal safe because
> -       qemu_timer_expired() can be called from a signal. */
>      pt = &ts->clock->active_timers;
>      for(;;) {
>          t = *pt;
> @@ -324,8 +322,6 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
>      qemu_del_timer(ts);
>  
>      /* add the timer in the sorted list */
> -    /* NOTE: this code must be signal safe because
> -       qemu_timer_expired() can be called from a signal. */
>      pt = &ts->clock->active_timers;
>      for(;;) {
>          t = *pt;
> 

If you fix the imprecision in the log:

Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Paolo Bonzini]

Il 05/07/2013 19:51, Jan Kiszka ha scritto:
> On 2013-07-05 14:39, Stefan Hajnoczi wrote:
>> This series makes the following functions thread-safe:
>>
>>   qemu_mod_timer_ns()
>>   qemu_mod_timer()
>>   qemu_del_timer()
>>   qemu_timer_pending()
>>
>> The following were already thread-safe:
>>
>>   qemu_free_timer()
>>   qemu_new_timer()
>>   qemu_timer_expired()
>>
>> Now it is possible to use QEMUTimer outside the QEMU global mutex.  Timer
>> callbacks are still invoked from the main loop.  If a thread wishes to run
>> timer callbacks it must use a thread-safe QEMUBH (which Ping Fan Liu is working
>> on).
> 
> What do you mean with this? We need main-loop independent timers for any
> task that depends on timely alarm delivery. Do your patches keep this in
> mind as well?

These are orthogonal issues.  Stefan's usecase does not need timely
delivery.

>> Note that host_clock is not thread-safe because it keeps state and invokes
>> reset notifiers.  Device emulation threads mostly care about vm_clock, so this
>> is not a problem.
> 
> I suppose you know that vm_clock cannot be read outside BQL yet due to
> timers_state and, under TCG, icount. Any ideas regarding this already? I
> didn't have to solve that problem so far as I only need CLOCK_REALTIME
> outside BQL.

I was thinking of a seqlock.  It should be quite cheap, since there
would be hardly any contention.

No ideas about host_clock's notifiers.

Paolo

Re: [Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Jan Kiszka]

On 2013-07-15 14:45, Paolo Bonzini wrote:
> Il 05/07/2013 19:51, Jan Kiszka ha scritto:
>> On 2013-07-05 14:39, Stefan Hajnoczi wrote:
>>> This series makes the following functions thread-safe:
>>>
>>>   qemu_mod_timer_ns()
>>>   qemu_mod_timer()
>>>   qemu_del_timer()
>>>   qemu_timer_pending()
>>>
>>> The following were already thread-safe:
>>>
>>>   qemu_free_timer()
>>>   qemu_new_timer()
>>>   qemu_timer_expired()
>>>
>>> Now it is possible to use QEMUTimer outside the QEMU global mutex.  Timer
>>> callbacks are still invoked from the main loop.  If a thread wishes to run
>>> timer callbacks it must use a thread-safe QEMUBH (which Ping Fan Liu is working
>>> on).
>>
>> What do you mean with this? We need main-loop independent timers for any
>> task that depends on timely alarm delivery. Do your patches keep this in
>> mind as well?
> 
> These are orthogonal issues.  Stefan's usecase does not need timely
> delivery.

Not necessarily. Timely delivery is likely the harder requirement that
also fulfills the need to move timer setup/manipulation outside of BQL.
I didn't have time to look into details yet, but there is a risk that
this rework will not help to achieve RT qualities but rather needs
another, non-orthogonal rework.

> 
>>> Note that host_clock is not thread-safe because it keeps state and invokes
>>> reset notifiers.  Device emulation threads mostly care about vm_clock, so this
>>> is not a problem.
>>
>> I suppose you know that vm_clock cannot be read outside BQL yet due to
>> timers_state and, under TCG, icount. Any ideas regarding this already? I
>> didn't have to solve that problem so far as I only need CLOCK_REALTIME
>> outside BQL.
> 
> I was thinking of a seqlock.  It should be quite cheap, since there
> would be hardly any contention.

Will think about this as well.

> 
> No ideas about host_clock's notifiers.

Hmm, we should be able to push the notification into BH context over the
main thread. Just jump detection requires the read context - unless we
want to poll for it.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Paolo Bonzini]

Il 15/07/2013 14:57, Jan Kiszka ha scritto:
> On 2013-07-15 14:45, Paolo Bonzini wrote:
>> Il 05/07/2013 19:51, Jan Kiszka ha scritto:
>>> On 2013-07-05 14:39, Stefan Hajnoczi wrote:
>>>> This series makes the following functions thread-safe:
>>>>
>>>>   qemu_mod_timer_ns()
>>>>   qemu_mod_timer()
>>>>   qemu_del_timer()
>>>>   qemu_timer_pending()
>>>>
>>>> The following were already thread-safe:
>>>>
>>>>   qemu_free_timer()
>>>>   qemu_new_timer()
>>>>   qemu_timer_expired()
>>>>
>>>> Now it is possible to use QEMUTimer outside the QEMU global mutex.  Timer
>>>> callbacks are still invoked from the main loop.  If a thread wishes to run
>>>> timer callbacks it must use a thread-safe QEMUBH (which Ping Fan Liu is working
>>>> on).
>>>
>>> What do you mean with this? We need main-loop independent timers for any
>>> task that depends on timely alarm delivery. Do your patches keep this in
>>> mind as well?
>>
>> These are orthogonal issues.  Stefan's usecase does not need timely
>> delivery.
> 
> Not necessarily. Timely delivery is likely the harder requirement that
> also fulfills the need to move timer setup/manipulation outside of BQL.
> I didn't have time to look into details yet, but there is a risk that
> this rework will not help to achieve RT qualities

Not a risk, a certainty. :)

> but rather needs another, non-orthogonal rework.

If you consider the "timers as a library" approach from your
presentation at last year's KVM Forum, this series does nothing to help
that goal.

But it also does nothing to hinder it (all it does is add a mutex,
basically), which is why I said it is orthogonal.

Paolo

Re: [Qemu-devel] [PATCH 0/3] qemu-timer: make QEMUTimer functions thread-safe

[Stefan Hajnoczi]

On Fri, Jul 05, 2013 at 02:39:43PM +0200, Stefan Hajnoczi wrote:
> This series makes the following functions thread-safe:
> 
>   qemu_mod_timer_ns()
>   qemu_mod_timer()
>   qemu_del_timer()
>   qemu_timer_pending()
> 
> The following were already thread-safe:
> 
>   qemu_free_timer()
>   qemu_new_timer()
>   qemu_timer_expired()
> 
> Now it is possible to use QEMUTimer outside the QEMU global mutex.  Timer
> callbacks are still invoked from the main loop.  If a thread wishes to run
> timer callbacks it must use a thread-safe QEMUBH (which Ping Fan Liu is working
> on).
> 
> Note that host_clock is not thread-safe because it keeps state and invokes
> reset notifiers.  Device emulation threads mostly care about vm_clock, so this
> is not a problem.
> 
> Stefan Hajnoczi (3):
>   qemu-timer: drop outdated signal safety comments
>   qemu-timer: add QEMUClock->active_timers list lock
>   qemu-timer: add qemu_alarm_timer->timer_modified_lock
> 
>  qemu-timer.c | 129 +++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 90 insertions(+), 39 deletions(-)

I'm rethinking this patch series.  If we go ahead and give every
AioContext a QEMUClock which also fires during aio_poll(), then
thread-safety is no longer an issue because:

1. Event loops like the dataplane thread will use the AioContext
   QEMUClock since are based purely on aio_poll().

2. The AioContext timers are implemented using g_poll()/ppoll() timeout
   in the same thread instead of global mechanisms (like signals).  It's
   not necessary to access timers from multiple threads.

Leaving this series for now unless someone needs thread-safe QEMUTimer,
in which case I can finish it off.

Stefan