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

2013-07-05 Thread 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 stefa...@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;
 }
+

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

2013-07-05 Thread 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 stefa...@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