[Qemu-devel] [PATCH 0/8] Make icount thread-safe

[Qemu-devel] [PATCH 0/8] Make icount thread-safe

[Paolo Bonzini]

This series moves the icount state under the same seqlock as the "normal"
vm_clock implementation.

It is not yet 100% thread-safe, because the CPU list should be moved
under RCU protection (due to the call to !all_cpu_threads_idle()
in qemu_clock_warp).  However it is a substantial step forward, the
only uncovered case being CPU hotplug.

Please review.

Paolo

Paolo Bonzini (8):
  timers: extract timer_mod_ns_locked and timerlist_rearm
  timers: add timer_mod_anticipate and timer_mod_anticipate_ns
  timers: use cpu_get_icount() directly
  timers: reorganize icount_warp_rt
  timers: prepare the code for future races in calling qemu_clock_warp
  timers: introduce cpu_get_clock_locked
  timers: document (future) locking rules for icount
  timers: make icount thread-safe

 cpus.c                 | 110 ++++++++++++++++++++++++++++++++++++-------------
 include/qemu/timer.h   |  26 +++++++++
 qemu-timer.c           |  74 +++++++++++++++++++------
 4 files changed, 163 insertions(+), 47 deletions(-)
 create mode 100644 include/qemu/seqlock.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/8] timers: extract timer_mod_ns_locked and timerlist_rearm

[Paolo Bonzini]

These will be reused in timer_mod_anticipate functions.

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

diff --git a/qemu-timer.c b/qemu-timer.c
index 6b62e88..95fc6eb 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -338,6 +338,34 @@ static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
     }
 }
 
+static bool timer_mod_ns_locked(QEMUTimerList *timer_list,
+                                QEMUTimer *ts, int64_t expire_time)
+{
+    QEMUTimer **pt, *t;
+
+    /* add the timer in the sorted list */
+    pt = &timer_list->active_timers;
+    for (;;) {
+        t = *pt;
+        if (!timer_expired_ns(t, expire_time)) {
+            break;
+        }
+        pt = &t->next;
+    }
+    ts->expire_time = MAX(expire_time, 0);
+    ts->next = *pt;
+    *pt = ts;
+
+    return pt == &timer_list->active_timers;
+}
+
+static void timerlist_rearm(QEMUTimerList *timer_list)
+{
+    /* Interrupt execution to force deadline recalculation.  */
+    qemu_clock_warp(timer_list->clock->type);
+    timerlist_notify(timer_list);
+}
+
 /* stop a timer, but do not dealloc it */
 void timer_del(QEMUTimer *ts)
 {
@@ -353,30 +381,15 @@ void timer_del(QEMUTimer *ts)
 void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
 {
     QEMUTimerList *timer_list = ts->timer_list;
-    QEMUTimer **pt, *t;
+    bool rearm;
 
     qemu_mutex_lock(&timer_list->active_timers_lock);
     timer_del_locked(timer_list, ts);
-
-    /* add the timer in the sorted list */
-    pt = &timer_list->active_timers;
-    for(;;) {
-        t = *pt;
-        if (!timer_expired_ns(t, expire_time)) {
-            break;
-        }
-        pt = &t->next;
-    }
-    ts->expire_time = MAX(expire_time, 0);
-    ts->next = *pt;
-    *pt = ts;
+    rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
     qemu_mutex_unlock(&timer_list->active_timers_lock);
 
-    /* Rearm if necessary  */
-    if (pt == &timer_list->active_timers) {
-        /* Interrupt execution to force deadline recalculation.  */
-        qemu_clock_warp(timer_list->clock->type);
-        timerlist_notify(timer_list);
+    if (rearm) {
+        timerlist_rearm(timer_list);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/8] timers: add timer_mod_anticipate and timer_mod_anticipate_ns

[Paolo Bonzini]

These let a user anticipate the deadline of a timer, atomically with
other sites that call the function.  This helps avoiding complicated
lock hierarchies.  It is useful whenever the timer does work based on
the current value of the clock (rather than doing something periodically
on every tick).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/timer.h | 26 ++++++++++++++++++++++++++
 qemu-timer.c         | 29 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index b58903b..f215b0b 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -539,6 +539,19 @@ void timer_del(QEMUTimer *ts);
 void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
 
 /**
+ * timer_mod_anticipate_ns:
+ * @ts: the timer
+ * @expire_time: the expiry time in nanoseconds
+ *
+ * Modify a timer to expire at @expire_time or the current time,
+ * whichever comes earlier.
+ *
+ * This function is thread-safe but the timer and its timer list must not be
+ * freed while this function is running.
+ */
+void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time);
+
+/**
  * timer_mod:
  * @ts: the timer
  * @expire_time: the expire time in the units associated with the timer
@@ -552,6 +565,19 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
 void timer_mod(QEMUTimer *ts, int64_t expire_timer);
 
 /**
+ * timer_mod_anticipate:
+ * @ts: the timer
+ * @expire_time: the expiry time in nanoseconds
+ *
+ * Modify a timer to expire at @expire_time or the current time, whichever
+ * comes earlier, 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_anticipate(QEMUTimer *ts, int64_t expire_time);
+
+/**
  * timer_pending:
  * @ts: the timer
  *
diff --git a/qemu-timer.c b/qemu-timer.c
index 95fc6eb..202e9a2 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -393,11 +393,40 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
     }
 }
 
+/* modify the current timer so that it will be fired when current_time
+   >= expire_time or the current deadline, whichever comes earlier.
+   The corresponding callback will be called. */
+void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time)
+{
+    QEMUTimerList *timer_list = ts->timer_list;
+    bool rearm;
+
+    qemu_mutex_lock(&timer_list->active_timers_lock);
+    if (ts->expire_time == -1 || ts->expire_time > expire_time) {
+        if (ts->expire_time != -1) {
+            timer_del_locked(timer_list, ts);
+        }
+        rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
+    } else {
+        rearm = false;
+    }
+    qemu_mutex_unlock(&timer_list->active_timers_lock);
+
+    if (rearm) {
+        timerlist_rearm(timer_list);
+    }
+}
+
 void timer_mod(QEMUTimer *ts, int64_t expire_time)
 {
     timer_mod_ns(ts, expire_time * ts->scale);
 }
 
+void timer_mod_anticipate(QEMUTimer *ts, int64_t expire_time)
+{
+    timer_mod_anticipate_ns(ts, expire_time * ts->scale);
+}
+
 bool timer_pending(QEMUTimer *ts)
 {
     return ts->expire_time >= 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/8] timers: use cpu_get_icount() directly

[Paolo Bonzini]

This will help later when we will have to place these calls in
a critical section, and thus call a version of cpu_get_icount()
that does not take the lock.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index 870a832..f87ff6f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -224,12 +224,15 @@ static void icount_adjust(void)
     int64_t cur_icount;
     int64_t delta;
     static int64_t last_delta;
+
     /* If the VM is not running, then do nothing.  */
     if (!runstate_is_running()) {
         return;
     }
+
     cur_time = cpu_get_clock();
-    cur_icount = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    cur_icount = cpu_get_icount();
+
     delta = cur_icount - cur_time;
     /* FIXME: This is a very crude algorithm, somewhat prone to oscillation.  */
     if (delta > 0
@@ -285,7 +288,7 @@ static void icount_warp_rt(void *opaque)
              * far ahead of real time.
              */
             int64_t cur_time = cpu_get_clock();
-            int64_t cur_icount = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            int64_t cur_icount = cpu_get_icount();
             int64_t delta = cur_time - cur_icount;
             qemu_icount_bias += MIN(warp_delta, delta);
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/8] timers: reorganize icount_warp_rt

[Paolo Bonzini]

To prepare for future code changes, move the increment of qemu_icount_bias
outside the "if" statement.

Also, hoist outside the if the check for timers that expired due to the
"warping".  The check is redundant when !runstate_is_running(), but
doing it this way helps because the code that increments qemu_icount_bias
will be a critical section.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/cpus.c b/cpus.c
index f87ff6f..9f450ad 100644
--- a/cpus.c
+++ b/cpus.c
@@ -279,10 +279,10 @@ static void icount_warp_rt(void *opaque)
 
     if (runstate_is_running()) {
         int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-        int64_t warp_delta = clock - vm_clock_warp_start;
-        if (use_icount == 1) {
-            qemu_icount_bias += warp_delta;
-        } else {
+        int64_t warp_delta;
+
+        warp_delta = clock - vm_clock_warp_start;
+        if (use_icount == 2) {
             /*
              * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
              * far ahead of real time.
@@ -290,13 +290,15 @@ static void icount_warp_rt(void *opaque)
             int64_t cur_time = cpu_get_clock();
             int64_t cur_icount = cpu_get_icount();
             int64_t delta = cur_time - cur_icount;
-            qemu_icount_bias += MIN(warp_delta, delta);
-        }
-        if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
-            qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+            warp_delta = MIN(warp_delta, delta);
         }
+        qemu_icount_bias += warp_delta;
     }
     vm_clock_warp_start = -1;
+
+    if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
+        qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+    }
 }
 
 void qtest_clock_warp(int64_t dest)
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp

[Paolo Bonzini]

Computing the deadline of all vm_clocks is somewhat expensive and calls
out to qemu-timer.c; two reasons not to do it in the seqlock's write-side
critical section.  This however opens the door for races in setting and
reading vm_clock_warp_start.

To plug them, we need to cover the case where a new deadline slips in
between the call to qemu_clock_deadline_ns_all and the actual modification
of the icount_warp_timer.  Restrict changes to vm_clock_warp_start and
the icount_warp_timer's expiration time, to only move them back (which
would simply cause an early wakeup).

If a vm_clock timer is cancelled while CPUs are idle, this might cause the
icount_warp_timer to fire unnecessarily.  This is not a problem, after it
fires the timer becomes inactive and the next call to timer_mod_anticipate
will be precise.

In addition to this, we must deactivate the icount_warp_timer _before_
checking whether CPUs are idle.  This way, if the "last" CPU becomes idle
during the call to timer_del we will still set up the icount_warp_timer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/cpus.c b/cpus.c
index 9f450ad..08eaf23 100644
--- a/cpus.c
+++ b/cpus.c
@@ -319,6 +319,7 @@ void qtest_clock_warp(int64_t dest)
 
 void qemu_clock_warp(QEMUClockType type)
 {
+    int64_t clock;
     int64_t deadline;
 
     /*
@@ -338,7 +339,7 @@ void qemu_clock_warp(QEMUClockType type)
      * the earliest QEMU_CLOCK_VIRTUAL timer.
      */
     icount_warp_rt(NULL);
-    if (!all_cpu_threads_idle() || !qemu_clock_has_timers(QEMU_CLOCK_VIRTUAL)) {
-        timer_del(icount_warp_timer);
+    timer_del(icount_warp_timer);
+    if (!all_cpu_threads_idle()) {
         return;
     }
@@ -348,17 +349,11 @@ void qemu_clock_warp(QEMUClockType type)
 	return;
     }
 
-    vm_clock_warp_start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     /* We want to use the earliest deadline from ALL vm_clocks */
+    clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
-
-    /* Maintain prior (possibly buggy) behaviour where if no deadline
-     * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
-     * INT32_MAX nanoseconds ahead, we still use INT32_MAX
-     * nanoseconds.
-     */
-    if ((deadline < 0) || (deadline > INT32_MAX)) {
-        deadline = INT32_MAX;
+    if (deadline < 0) {
+        return;
     }
 
     if (deadline > 0) {
@@ -379,7 +375,10 @@ void qemu_clock_warp(QEMUClockType type)
          * you will not be sending network packets continuously instead of
          * every 100ms.
          */
-        timer_mod(icount_warp_timer, vm_clock_warp_start + deadline);
+        if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
+            vm_clock_warp_start = clock;
+        }
+        timer_mod_anticipate(icount_warp_timer, clock + deadline);
     } else if (deadline == 0) {
         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/8] timers: introduce cpu_get_clock_locked

[Paolo Bonzini]

This fixes a deadlock in cpu_disable_ticks.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Should be squashed in Ping Fan's patches.

 cpus.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index 08eaf23..01acce2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -166,6 +166,20 @@ int64_t cpu_get_ticks(void)
     }
 }
 
+static int64_t cpu_get_clock_locked(void)
+{
+    int64_t ti;
+
+    if (!timers_state.cpu_ticks_enabled) {
+        ti = timers_state.cpu_clock_offset;
+    } else {
+        ti = get_clock();
+        ti += timers_state.cpu_clock_offset;
+    }
+
+    return ti;
+}
+
 /* return the host CPU monotonic timer and handle stop/restart */
 int64_t cpu_get_clock(void)
 {
@@ -174,12 +188,7 @@ int64_t cpu_get_clock(void)
 
     do {
         start = seqlock_read_begin(&timers_state.clock_seqlock);
-        if (!timers_state.cpu_ticks_enabled) {
-            ti = timers_state.cpu_clock_offset;
-        } else {
-            ti = get_clock();
-            ti += timers_state.cpu_clock_offset;
-        }
+        ti = cpu_get_clock_locked();
     } while (seqlock_read_retry(&timers_state.clock_seqlock, start));
 
     return ti;
@@ -220,7 +233,7 @@ void cpu_disable_ticks(void)
     seqlock_write_lock(&timers_state.clock_seqlock);
     if (timers_state.cpu_ticks_enabled) {
         timers_state.cpu_ticks_offset = cpu_get_ticks();
-        timers_state.cpu_clock_offset = cpu_get_clock();
+        timers_state.cpu_clock_offset = cpu_get_clock_locked();
         timers_state.cpu_ticks_enabled = 0;
     }
     seqlock_write_unlock(&timers_state.clock_seqlock);
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/8] timers: document (future) locking rules for icount

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index 01acce2..bc675a4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -98,17 +98,22 @@ static bool all_cpu_threads_idle(void)
 /***********************************************************/
 /* guest cycle counter */
 
+/* Protected by TimersState seqlock */
+
+/* Compensate for varying guest execution speed.  */
+static int64_t qemu_icount_bias;
+static int64_t vm_clock_warp_start;
 /* Conversion factor from emulated instructions to virtual clock ticks.  */
 static int icount_time_shift;
 /* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
 #define MAX_ICOUNT_SHIFT 10
-/* Compensate for varying guest execution speed.  */
-static int64_t qemu_icount_bias;
+
+/* Only written by TCG thread */
+static int64_t qemu_icount;
+
 static QEMUTimer *icount_rt_timer;
 static QEMUTimer *icount_vm_timer;
 static QEMUTimer *icount_warp_timer;
-static int64_t vm_clock_warp_start;
-static int64_t qemu_icount;
 
 typedef struct TimersState {
     int64_t cpu_ticks_prev;
@@ -232,6 +237,8 @@ static void icount_adjust(void)
     int64_t cur_time;
     int64_t cur_icount;
     int64_t delta;
+
+    /* Protected by TimersState mutex.  */
     static int64_t last_delta;
 
     /* If the VM is not running, then do nothing.  */
-- 
1.8.3.1

[Qemu-devel] [PATCH 8/8] timers: make icount thread-safe

[Paolo Bonzini]

This lets threads other than the I/O thread use vm_clock even in -icount mode.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/cpus.c b/cpus.c
index bc675a4..1e5cba4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -133,7 +133,7 @@ typedef struct TimersState {
 static TimersState timers_state;
 
 /* Return the virtual CPU time, based on the instruction counter.  */
-int64_t cpu_get_icount(void)
+static int64_t cpu_get_icount_locked(void)
 {
     int64_t icount;
     CPUState *cpu = current_cpu;
@@ -149,6 +149,19 @@ int64_t cpu_get_icount(void)
     return qemu_icount_bias + (icount << icount_time_shift);
 }
 
+int64_t cpu_get_icount(void)
+{
+    int64_t icount;
+    unsigned start;
+
+    do {
+        start = seqlock_read_begin(&timers_state.clock_seqlock);
+        icount = cpu_get_icount_locked();
+    } while (seqlock_read_retry(&timers_state.clock_seqlock, start));
+
+    return icount;
+}
+
 /* return the host CPU cycle counter and handle stop/restart */
 /* cpu_ticks is safely if holding BQL */
 int64_t cpu_get_ticks(void)
@@ -246,8 +259,9 @@ static void icount_adjust(void)
         return;
     }
 
-    cur_time = cpu_get_clock();
-    cur_icount = cpu_get_icount();
+    seqlock_write_lock(&timers_state.clock_seqlock);
+    cur_time = cpu_get_clock_locked();
+    cur_icount = cpu_get_icount_locked();
 
     delta = cur_icount - cur_time;
     /* FIXME: This is a very crude algorithm, somewhat prone to oscillation.  */
@@ -265,6 +279,7 @@ static void icount_adjust(void)
     }
     last_delta = delta;
     qemu_icount_bias = cur_icount - (qemu_icount << icount_time_shift);
+    seqlock_write_unlock(&timers_state.clock_seqlock);
 }
 
 static void icount_adjust_rt(void *opaque)
@@ -289,10 +304,14 @@ static int64_t qemu_icount_round(int64_t count)
 
 static void icount_warp_rt(void *opaque)
 {
-    if (vm_clock_warp_start == -1) {
+    /* The icount_warp_timer is rescheduled soon after vm_clock_warp_start
+     * changes from -1 to another value, so the race here is okay.
+     */
+    if (atomic_read(&vm_clock_warp_start) == -1) {
         return;
     }
 
+    seqlock_write_lock(&timers_state.clock_seqlock);
     if (runstate_is_running()) {
         int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
         int64_t warp_delta;
@@ -303,14 +322,15 @@ static void icount_warp_rt(void *opaque)
              * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
              * far ahead of real time.
              */
-            int64_t cur_time = cpu_get_clock();
-            int64_t cur_icount = cpu_get_icount();
+            int64_t cur_time = cpu_get_clock_locked();
+            int64_t cur_icount = cpu_get_icount_locked();
             int64_t delta = cur_time - cur_icount;
             warp_delta = MIN(warp_delta, delta);
         }
         qemu_icount_bias += warp_delta;
     }
     vm_clock_warp_start = -1;
+    seqlock_write_unlock(&timers_state.clock_seqlock);
 
     if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
@@ -324,7 +344,10 @@ void qtest_clock_warp(int64_t dest)
     while (clock < dest) {
         int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
         int64_t warp = MIN(dest - clock, deadline);
+        seqlock_write_lock(&timers_state.clock_seqlock);
         qemu_icount_bias += warp;
+        seqlock_write_unlock(&timers_state.clock_seqlock);
+
         qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
         clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     }
@@ -391,9 +415,11 @@ void qemu_clock_warp(QEMUClockType type)
          * you will not be sending network packets continuously instead of
          * every 100ms.
          */
+        seqlock_write_lock(&timers_state.clock_seqlock);
         if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
             vm_clock_warp_start = clock;
         }
+        seqlock_write_unlock(&timers_state.clock_seqlock);
         timer_mod_anticipate(icount_warp_timer, clock + deadline);
     } else if (deadline == 0) {
         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/8] timers: extract timer_mod_ns_locked and timerlist_rearm

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> These will be reused in timer_mod_anticipate functions.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
> qemu-timer.c | 51 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 6b62e88..95fc6eb 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -338,6 +338,34 @@ static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
>     }
> }
> 
> +static bool timer_mod_ns_locked(QEMUTimerList *timer_list,
> +                                QEMUTimer *ts, int64_t expire_time)
> +{
> +    QEMUTimer **pt, *t;
> +
> +    /* add the timer in the sorted list */
> +    pt = &timer_list->active_timers;
> +    for (;;) {
> +        t = *pt;
> +        if (!timer_expired_ns(t, expire_time)) {
> +            break;
> +        }
> +        pt = &t->next;
> +    }
> +    ts->expire_time = MAX(expire_time, 0);
> +    ts->next = *pt;
> +    *pt = ts;
> +
> +    return pt == &timer_list->active_timers;
> +}
> +
> +static void timerlist_rearm(QEMUTimerList *timer_list)
> +{
> +    /* Interrupt execution to force deadline recalculation.  */
> +    qemu_clock_warp(timer_list->clock->type);
> +    timerlist_notify(timer_list);
> +}
> +
> /* stop a timer, but do not dealloc it */
> void timer_del(QEMUTimer *ts)
> {
> @@ -353,30 +381,15 @@ void timer_del(QEMUTimer *ts)
> void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
> {
>     QEMUTimerList *timer_list = ts->timer_list;
> -    QEMUTimer **pt, *t;
> +    bool rearm;
> 
>     qemu_mutex_lock(&timer_list->active_timers_lock);
>     timer_del_locked(timer_list, ts);
> -
> -    /* add the timer in the sorted list */
> -    pt = &timer_list->active_timers;
> -    for(;;) {
> -        t = *pt;
> -        if (!timer_expired_ns(t, expire_time)) {
> -            break;
> -        }
> -        pt = &t->next;
> -    }
> -    ts->expire_time = MAX(expire_time, 0);
> -    ts->next = *pt;
> -    *pt = ts;
> +    rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
>     qemu_mutex_unlock(&timer_list->active_timers_lock);
> 
> -    /* Rearm if necessary  */
> -    if (pt == &timer_list->active_timers) {
> -        /* Interrupt execution to force deadline recalculation.  */
> -        qemu_clock_warp(timer_list->clock->type);
> -        timerlist_notify(timer_list);
> +    if (rearm) {
> +        timerlist_rearm(timer_list);
>     }
> }
> 
> -- 
> 1.8.3.1
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 2/8] timers: add timer_mod_anticipate and timer_mod_anticipate_ns

[Alex Bligh]

Paolo,

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:
> 
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -393,11 +393,40 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
>     }
> }
> 
> +/* modify the current timer so that it will be fired when current_time
> +   >= expire_time or the current deadline, whichever comes earlier.
> +   The corresponding callback will be called. */
> +void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time)
> +{
> +    QEMUTimerList *timer_list = ts->timer_list;
> +    bool rearm;
> +
> +    qemu_mutex_lock(&timer_list->active_timers_lock);
> +    if (ts->expire_time == -1 || ts->expire_time > expire_time) {

So "if we want to alter it" ...

> +        if (ts->expire_time != -1) {
> +            timer_del_locked(timer_list, ts);
> +        }

What's this bit for? Surely you've calculated whether you are
shortening the expiry time (above), so all you need do now is
modify it. Why delete it? timer_mod_ns doesn't make this
check?

Otherwise looks OK.

> +        rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
> +    } else {
> +        rearm = false;
> +    }
> +    qemu_mutex_unlock(&timer_list->active_timers_lock);
> +
> +    if (rearm) {
> +        timerlist_rearm(timer_list);
> +    }
> +}
> +
> void timer_mod(QEMUTimer *ts, int64_t expire_time)
> {
>     timer_mod_ns(ts, expire_time * ts->scale);
> }
> 
> +void timer_mod_anticipate(QEMUTimer *ts, int64_t expire_time)
> +{
> +    timer_mod_anticipate_ns(ts, expire_time * ts->scale);
> +}
> +
> bool timer_pending(QEMUTimer *ts)
> {
>     return ts->expire_time >= 0;
> -- 
> 1.8.3.1
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 2/8] timers: add timer_mod_anticipate and timer_mod_anticipate_ns

[Paolo Bonzini]

Il 08/10/2013 11:15, Alex Bligh ha scritto:
> So "if we want to alter it" ...
> 
>> > +        if (ts->expire_time != -1) {
>> > +            timer_del_locked(timer_list, ts);
>> > +        }
> What's this bit for? Surely you've calculated whether you are
> shortening the expiry time (above), so all you need do now is
> modify it. Why delete it? timer_mod_ns doesn't make this
> check?

timer_mod_ns_locked does not remove the timer from the list:

    qemu_mutex_lock(&timer_list->active_timers_lock);
    timer_del_locked(timer_list, ts);
    rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
    qemu_mutex_unlock(&timer_list->active_timers_lock);

This is doing the same.  The check in the "if" is not strictly
necessary, but it saves a useless visit of the list.  It could be added
to timer_mod_ns as well.

Paolo

Re: [Qemu-devel] [PATCH 0/8] Make icount thread-safe

[Andreas Färber]

Am 08.10.2013 10:47, schrieb Paolo Bonzini:
> This series moves the icount state under the same seqlock as the "normal"
> vm_clock implementation.
> 
> It is not yet 100% thread-safe, because the CPU list should be moved
> under RCU protection (due to the call to !all_cpu_threads_idle()
> in qemu_clock_warp).  However it is a substantial step forward, the
> only uncovered case being CPU hotplug.
> 
> Please review.
> 
> Paolo
> 
> Paolo Bonzini (8):
>   timers: extract timer_mod_ns_locked and timerlist_rearm
>   timers: add timer_mod_anticipate and timer_mod_anticipate_ns

>   timers: use cpu_get_icount() directly
>   timers: reorganize icount_warp_rt
>   timers: prepare the code for future races in calling qemu_clock_warp
>   timers: introduce cpu_get_clock_locked
>   timers: document (future) locking rules for icount
>   timers: make icount thread-safe

These patches touch cpus.c exclusively, so "timers:" is rather misleading.

As you know I have pending patches (in need of rebase due to the
performance issue you raised) moving the icount CPU fields around.
Is there anything in particular I should be aware of? Looks to me as if
this may be orthogonal?

What about the previous patch disabling icount for -smp? Does this
series supersede it or does it fix different concurrency issues?

Thanks,
Andreas

>  cpus.c                 | 110 ++++++++++++++++++++++++++++++++++++-------------
>  include/qemu/timer.h   |  26 +++++++++
>  qemu-timer.c           |  74 +++++++++++++++++++------
>  4 files changed, 163 insertions(+), 47 deletions(-)
>  create mode 100644 include/qemu/seqlock.h

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/8] Make icount thread-safe

[Paolo Bonzini]

Il 08/10/2013 15:47, Andreas Färber ha scritto:
> Am 08.10.2013 10:47, schrieb Paolo Bonzini:
>> This series moves the icount state under the same seqlock as the "normal"
>> vm_clock implementation.
>>
>> It is not yet 100% thread-safe, because the CPU list should be moved
>> under RCU protection (due to the call to !all_cpu_threads_idle()
>> in qemu_clock_warp).  However it is a substantial step forward, the
>> only uncovered case being CPU hotplug.
>>
>> Please review.
>>
>> Paolo
>>
>> Paolo Bonzini (8):
>>   timers: extract timer_mod_ns_locked and timerlist_rearm
>>   timers: add timer_mod_anticipate and timer_mod_anticipate_ns
> 
>>   timers: use cpu_get_icount() directly
>>   timers: reorganize icount_warp_rt
>>   timers: prepare the code for future races in calling qemu_clock_warp
>>   timers: introduce cpu_get_clock_locked
>>   timers: document (future) locking rules for icount
>>   timers: make icount thread-safe
> 
> These patches touch cpus.c exclusively, so "timers:" is rather misleading.

I can change that to "icount".

> As you know I have pending patches (in need of rebase due to the
> performance issue you raised) moving the icount CPU fields around.
> Is there anything in particular I should be aware of? Looks to me as if
> this may be orthogonal?

It's entirely orthogonal.  It doesn't affect the cpu-exec part of
icount, only the "timers" :) part.

> What about the previous patch disabling icount for -smp? Does this
> series supersede it or does it fix different concurrency issues?

This is for making accesses to icount safe without holding the BQL.
icount for -smp remains just as broken as before.

Paolo

Re: [Qemu-devel] [PATCH 3/8] timers: use cpu_get_icount() directly

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> This will help later when we will have to place these calls in
> a critical section, and thus call a version of cpu_get_icount()
> that does not take the lock.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-By: Alex Bligh <alex@alex.org.uk>

(and I meant Reviewed-By: in patches 1 and 2 - thanks Paolo
for setting me straight)

> ---
> cpus.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 870a832..f87ff6f 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -224,12 +224,15 @@ static void icount_adjust(void)
>     int64_t cur_icount;
>     int64_t delta;
>     static int64_t last_delta;
> +
>     /* If the VM is not running, then do nothing.  */
>     if (!runstate_is_running()) {
>         return;
>     }
> +
>     cur_time = cpu_get_clock();
> -    cur_icount = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    cur_icount = cpu_get_icount();
> +
>     delta = cur_icount - cur_time;
>     /* FIXME: This is a very crude algorithm, somewhat prone to oscillation.  */
>     if (delta > 0
> @@ -285,7 +288,7 @@ static void icount_warp_rt(void *opaque)
>              * far ahead of real time.
>              */
>             int64_t cur_time = cpu_get_clock();
> -            int64_t cur_icount = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +            int64_t cur_icount = cpu_get_icount();
>             int64_t delta = cur_time - cur_icount;
>             qemu_icount_bias += MIN(warp_delta, delta);
>         }
> -- 
> 1.8.3.1
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 4/8] timers: reorganize icount_warp_rt

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> To prepare for future code changes, move the increment of qemu_icount_bias
> outside the "if" statement.
> 
> Also, hoist outside the if the check for timers that expired due to the
> "warping".  The check is redundant when !runstate_is_running(), but
> doing it this way helps because the code that increments qemu_icount_bias
> will be a critical section.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> cpus.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)

Looks good - too much icount mindbending for Reviewed-By:

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> Computing the deadline of all vm_clocks is somewhat expensive and calls
> out to qemu-timer.c; two reasons not to do it in the seqlock's write-side
> critical section.  This however opens the door for races in setting and
> reading vm_clock_warp_start.
> 
> To plug them, we need to cover the case where a new deadline slips in
> between the call to qemu_clock_deadline_ns_all and the actual modification
> of the icount_warp_timer.  Restrict changes to vm_clock_warp_start and
> the icount_warp_timer's expiration time, to only move them back (which
> would simply cause an early wakeup).
> 
> If a vm_clock timer is cancelled while CPUs are idle, this might cause the
> icount_warp_timer to fire unnecessarily.  This is not a problem, after it
> fires the timer becomes inactive and the next call to timer_mod_anticipate
> will be precise.
> 
> In addition to this, we must deactivate the icount_warp_timer _before_
> checking whether CPUs are idle.  This way, if the "last" CPU becomes idle
> during the call to timer_del we will still set up the icount_warp_timer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> cpus.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 9f450ad..08eaf23 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -319,6 +319,7 @@ void qtest_clock_warp(int64_t dest)
> 
> void qemu_clock_warp(QEMUClockType type)
> {
> +    int64_t clock;
>     int64_t deadline;
> 
>     /*
> @@ -338,7 +339,7 @@ void qemu_clock_warp(QEMUClockType type)
>      * the earliest QEMU_CLOCK_VIRTUAL timer.
>      */
>     icount_warp_rt(NULL);
> -    if (!all_cpu_threads_idle() || !qemu_clock_has_timers(QEMU_CLOCK_VIRTUAL)) {
> -        timer_del(icount_warp_timer);
> +    timer_del(icount_warp_timer);
> +    if (!all_cpu_threads_idle()) {
>         return;
>     }
> @@ -348,17 +349,11 @@ void qemu_clock_warp(QEMUClockType type)
> 	return;
>     }
> 
> -    vm_clock_warp_start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>     /* We want to use the earliest deadline from ALL vm_clocks */
> +    clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>     deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
> -
> -    /* Maintain prior (possibly buggy) behaviour where if no deadline
> -     * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
> -     * INT32_MAX nanoseconds ahead, we still use INT32_MAX
> -     * nanoseconds.
> -     */
> -    if ((deadline < 0) || (deadline > INT32_MAX)) {
> -        deadline = INT32_MAX;
> +    if (deadline < 0) {
> +        return;
>     }

Arguably the patch could document why removing the check for deadline > INT32_MAX
(the bug for bug compatibility) is safe, as I couldn't entirely convince myself it
was, mostly because I couldn't see why it was doing it in the first place.

> 
>     if (deadline > 0) {
> @@ -379,7 +375,10 @@ void qemu_clock_warp(QEMUClockType type)
>          * you will not be sending network packets continuously instead of
>          * every 100ms.
>          */
> -        timer_mod(icount_warp_timer, vm_clock_warp_start + deadline);
> +        if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
> +            vm_clock_warp_start = clock;
> +        }
> +        timer_mod_anticipate(icount_warp_timer, clock + deadline);
>     } else if (deadline == 0) {
>         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>     }
> -- 
> 1.8.3.1
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 6/8] timers: introduce cpu_get_clock_locked

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> This fixes a deadlock in cpu_disable_ticks.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

> ---
> 	Should be squashed in Ping Fan's patches.
> 
> cpus.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 08eaf23..01acce2 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -166,6 +166,20 @@ int64_t cpu_get_ticks(void)
>     }
> }
> 
> +static int64_t cpu_get_clock_locked(void)
> +{
> +    int64_t ti;
> +
> +    if (!timers_state.cpu_ticks_enabled) {
> +        ti = timers_state.cpu_clock_offset;
> +    } else {
> +        ti = get_clock();
> +        ti += timers_state.cpu_clock_offset;
> +    }
> +
> +    return ti;
> +}
> +
> /* return the host CPU monotonic timer and handle stop/restart */
> int64_t cpu_get_clock(void)
> {
> @@ -174,12 +188,7 @@ int64_t cpu_get_clock(void)
> 
>     do {
>         start = seqlock_read_begin(&timers_state.clock_seqlock);
> -        if (!timers_state.cpu_ticks_enabled) {
> -            ti = timers_state.cpu_clock_offset;
> -        } else {
> -            ti = get_clock();
> -            ti += timers_state.cpu_clock_offset;
> -        }
> +        ti = cpu_get_clock_locked();
>     } while (seqlock_read_retry(&timers_state.clock_seqlock, start));
> 
>     return ti;
> @@ -220,7 +233,7 @@ void cpu_disable_ticks(void)
>     seqlock_write_lock(&timers_state.clock_seqlock);
>     if (timers_state.cpu_ticks_enabled) {
>         timers_state.cpu_ticks_offset = cpu_get_ticks();
> -        timers_state.cpu_clock_offset = cpu_get_clock();
> +        timers_state.cpu_clock_offset = cpu_get_clock_locked();
>         timers_state.cpu_ticks_enabled = 0;
>     }
>     seqlock_write_unlock(&timers_state.clock_seqlock);
> -- 
> 1.8.3.1
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 7/8] timers: document (future) locking rules for icount

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

> ---
> cpus.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 01acce2..bc675a4 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -98,17 +98,22 @@ static bool all_cpu_threads_idle(void)
> /***********************************************************/
> /* guest cycle counter */
> 
> +/* Protected by TimersState seqlock */
> +
> +/* Compensate for varying guest execution speed.  */
> +static int64_t qemu_icount_bias;
> +static int64_t vm_clock_warp_start;
> /* Conversion factor from emulated instructions to virtual clock ticks.  */
> static int icount_time_shift;
> /* Arbitrarily pick 1MIPS as the minimum allowable speed.  */
> #define MAX_ICOUNT_SHIFT 10
> -/* Compensate for varying guest execution speed.  */
> -static int64_t qemu_icount_bias;
> +
> +/* Only written by TCG thread */
> +static int64_t qemu_icount;
> +
> static QEMUTimer *icount_rt_timer;
> static QEMUTimer *icount_vm_timer;
> static QEMUTimer *icount_warp_timer;
> -static int64_t vm_clock_warp_start;
> -static int64_t qemu_icount;
> 
> typedef struct TimersState {
>     int64_t cpu_ticks_prev;
> @@ -232,6 +237,8 @@ static void icount_adjust(void)
>     int64_t cur_time;
>     int64_t cur_icount;
>     int64_t delta;
> +
> +    /* Protected by TimersState mutex.  */
>     static int64_t last_delta;
> 
>     /* If the VM is not running, then do nothing.  */
> -- 
> 1.8.3.1
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp

[Paolo Bonzini]

Il 08/10/2013 18:54, Alex Bligh ha scritto:
>> > -
>> > -    /* Maintain prior (possibly buggy) behaviour where if no deadline
>> > -     * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
>> > -     * INT32_MAX nanoseconds ahead, we still use INT32_MAX
>> > -     * nanoseconds.
>> > -     */
>> > -    if ((deadline < 0) || (deadline > INT32_MAX)) {
>> > -        deadline = INT32_MAX;
>> > +    if (deadline < 0) {
>> > +        return;
>> >     }
> 
> Arguably the patch could document why removing the check for deadline > INT32_MAX
> (the bug for bug compatibility) is safe, as I couldn't entirely convince myself it
> was, mostly because I couldn't see why it was doing it in the first place.

I couldn't convince myself that it is _not_ safe :) and it made the code
more complicated.  As soon as a deadline appears, qemu_clock_warp() will
be called again and update the icount_warp_timer.

Ok to move that to a separate patch?

Paolo

Re: [Qemu-devel] [PATCH 8/8] timers: make icount thread-safe

[Alex Bligh]

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:

> This lets threads other than the I/O thread use vm_clock even in -icount mode.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Looks good to me

Alex

> ---
> cpus.c | 42 ++++++++++++++++++++++++++++++++++--------
> 1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index bc675a4..1e5cba4 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -133,7 +133,7 @@ typedef struct TimersState {
> static TimersState timers_state;
> 
> /* Return the virtual CPU time, based on the instruction counter.  */
> -int64_t cpu_get_icount(void)
> +static int64_t cpu_get_icount_locked(void)
> {
>     int64_t icount;
>     CPUState *cpu = current_cpu;
> @@ -149,6 +149,19 @@ int64_t cpu_get_icount(void)
>     return qemu_icount_bias + (icount << icount_time_shift);
> }
> 
> +int64_t cpu_get_icount(void)
> +{
> +    int64_t icount;
> +    unsigned start;
> +
> +    do {
> +        start = seqlock_read_begin(&timers_state.clock_seqlock);
> +        icount = cpu_get_icount_locked();
> +    } while (seqlock_read_retry(&timers_state.clock_seqlock, start));
> +
> +    return icount;
> +}
> +
> /* return the host CPU cycle counter and handle stop/restart */
> /* cpu_ticks is safely if holding BQL */
> int64_t cpu_get_ticks(void)
> @@ -246,8 +259,9 @@ static void icount_adjust(void)
>         return;
>     }
> 
> -    cur_time = cpu_get_clock();
> -    cur_icount = cpu_get_icount();
> +    seqlock_write_lock(&timers_state.clock_seqlock);
> +    cur_time = cpu_get_clock_locked();
> +    cur_icount = cpu_get_icount_locked();
> 
>     delta = cur_icount - cur_time;
>     /* FIXME: This is a very crude algorithm, somewhat prone to oscillation.  */
> @@ -265,6 +279,7 @@ static void icount_adjust(void)
>     }
>     last_delta = delta;
>     qemu_icount_bias = cur_icount - (qemu_icount << icount_time_shift);
> +    seqlock_write_unlock(&timers_state.clock_seqlock);
> }
> 
> static void icount_adjust_rt(void *opaque)
> @@ -289,10 +304,14 @@ static int64_t qemu_icount_round(int64_t count)
> 
> static void icount_warp_rt(void *opaque)
> {
> -    if (vm_clock_warp_start == -1) {
> +    /* The icount_warp_timer is rescheduled soon after vm_clock_warp_start
> +     * changes from -1 to another value, so the race here is okay.
> +     */
> +    if (atomic_read(&vm_clock_warp_start) == -1) {
>         return;
>     }
> 
> +    seqlock_write_lock(&timers_state.clock_seqlock);
>     if (runstate_is_running()) {
>         int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>         int64_t warp_delta;
> @@ -303,14 +322,15 @@ static void icount_warp_rt(void *opaque)
>              * In adaptive mode, do not let QEMU_CLOCK_VIRTUAL run too
>              * far ahead of real time.
>              */
> -            int64_t cur_time = cpu_get_clock();
> -            int64_t cur_icount = cpu_get_icount();
> +            int64_t cur_time = cpu_get_clock_locked();
> +            int64_t cur_icount = cpu_get_icount_locked();
>             int64_t delta = cur_time - cur_icount;
>             warp_delta = MIN(warp_delta, delta);
>         }
>         qemu_icount_bias += warp_delta;
>     }
>     vm_clock_warp_start = -1;
> +    seqlock_write_unlock(&timers_state.clock_seqlock);
> 
>     if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) {
>         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
> @@ -324,7 +344,10 @@ void qtest_clock_warp(int64_t dest)
>     while (clock < dest) {
>         int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>         int64_t warp = MIN(dest - clock, deadline);
> +        seqlock_write_lock(&timers_state.clock_seqlock);
>         qemu_icount_bias += warp;
> +        seqlock_write_unlock(&timers_state.clock_seqlock);
> +
>         qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
>         clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>     }
> @@ -391,9 +415,11 @@ void qemu_clock_warp(QEMUClockType type)
>          * you will not be sending network packets continuously instead of
>          * every 100ms.
>          */
> +        seqlock_write_lock(&timers_state.clock_seqlock);
>         if (vm_clock_warp_start == -1 || vm_clock_warp_start > clock) {
>             vm_clock_warp_start = clock;
>         }
> +        seqlock_write_unlock(&timers_state.clock_seqlock);
>         timer_mod_anticipate(icount_warp_timer, clock + deadline);
>     } else if (deadline == 0) {
>         qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
> -- 
> 1.8.3.1
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 2/8] timers: add timer_mod_anticipate and timer_mod_anticipate_ns

[Alex Bligh]

On 8 Oct 2013, at 10:25, Paolo Bonzini wrote:

> Il 08/10/2013 11:15, Alex Bligh ha scritto:
>> So "if we want to alter it" ...
>> 
>>>> +        if (ts->expire_time != -1) {
>>>> +            timer_del_locked(timer_list, ts);
>>>> +        }
>> What's this bit for? Surely you've calculated whether you are
>> shortening the expiry time (above), so all you need do now is
>> modify it. Why delete it? timer_mod_ns doesn't make this
>> check?
> 
> timer_mod_ns_locked does not remove the timer from the list:
> 
>    qemu_mutex_lock(&timer_list->active_timers_lock);
>    timer_del_locked(timer_list, ts);
>    rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
>    qemu_mutex_unlock(&timer_list->active_timers_lock);
> 
> This is doing the same.  The check in the "if" is not strictly
> necessary, but it saves a useless visit of the list.  It could be added
> to timer_mod_ns as well.

Quite right. I'd missed the timer_del somehow.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp

[Alex Bligh]

On 8 Oct 2013, at 17:56, Paolo Bonzini wrote:

>> Arguably the patch could document why removing the check for deadline > INT32_MAX
>> (the bug for bug compatibility) is safe, as I couldn't entirely convince myself it
>> was, mostly because I couldn't see why it was doing it in the first place.
> 
> I couldn't convince myself that it is _not_ safe :) and it made the code
> more complicated.  As soon as a deadline appears, qemu_clock_warp() will
> be called again and update the icount_warp_timer.
> 
> Ok to move that to a separate patch?

To be honest I put it in out of an abundance of caution. I am very
tempted to take it out and see what breaks. As far as I can see all
the time arithmetic is not int64_t; perhaps this was not always the
case. I was more checking you hadn't removed it by accident. Perhaps
just add "special casing deadlines > INT32_MAX removed as all
arithmetic now 64 bit".

There is another offender in tcg_cpu_exec.

        deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);

        /* Maintain prior (possibly buggy) behaviour where if no deadline
         * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
         * INT32_MAX nanoseconds ahead, we still use INT32_MAX
         * nanoseconds.
         */
        if ((deadline < 0) || (deadline > INT32_MAX)) {
            deadline = INT32_MAX;
        }

        count = qemu_icount_round(deadline);
        qemu_icount += count;
        decr = (count > 0xffff) ? 0xffff : count;
        count -= decr;
        env->icount_decr.u16.low = decr;
        env->icount_extra = count;

This implies that qemu_icount_round() cannot take a 64 bit int.

static int64_t qemu_icount_round(int64_t count)
{
    return (count + (1 << icount_time_shift) - 1) >> icount_time_shift;
}

I would have thought it better if qemu_icount_round just
dealt sensibly with negative parameters.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp

[Paolo Bonzini]

Il 08/10/2013 19:08, Alex Bligh ha scritto:
> 
> On 8 Oct 2013, at 17:56, Paolo Bonzini wrote:
> 
>>> Arguably the patch could document why removing the check for deadline > INT32_MAX
>>> (the bug for bug compatibility) is safe, as I couldn't entirely convince myself it
>>> was, mostly because I couldn't see why it was doing it in the first place.
>>
>> I couldn't convince myself that it is _not_ safe :) and it made the code
>> more complicated.  As soon as a deadline appears, qemu_clock_warp() will
>> be called again and update the icount_warp_timer.
>>
>> Ok to move that to a separate patch?
> 
> To be honest I put it in out of an abundance of caution. I am very
> tempted to take it out and see what breaks. As far as I can see all
> the time arithmetic is not int64_t; perhaps this was not always the
> case. I was more checking you hadn't removed it by accident. Perhaps
> just add "special casing deadlines > INT32_MAX removed as all
> arithmetic now 64 bit".
> 
> There is another offender in tcg_cpu_exec.
> 
>         deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
> 
>         /* Maintain prior (possibly buggy) behaviour where if no deadline
>          * was set (as there is no QEMU_CLOCK_VIRTUAL timer) or it is more than
>          * INT32_MAX nanoseconds ahead, we still use INT32_MAX
>          * nanoseconds.
>          */
>         if ((deadline < 0) || (deadline > INT32_MAX)) {
>             deadline = INT32_MAX;
>         }
> 
>         count = qemu_icount_round(deadline);
>         qemu_icount += count;
>         decr = (count > 0xffff) ? 0xffff : count;
>         count -= decr;
>         env->icount_decr.u16.low = decr;
>         env->icount_extra = count;
> 
> This implies that qemu_icount_round() cannot take a 64 bit int.
> 
> static int64_t qemu_icount_round(int64_t count)
> {
>     return (count + (1 << icount_time_shift) - 1) >> icount_time_shift;
> }
> 
> I would have thought it better if qemu_icount_round just
> dealt sensibly with negative parameters.
> 

I'll clean that up separately.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/8] Make icount thread-safe

[Stefan Hajnoczi]

On Tue, Oct 08, 2013 at 10:47:30AM +0200, Paolo Bonzini wrote:
> This series moves the icount state under the same seqlock as the "normal"
> vm_clock implementation.
> 
> It is not yet 100% thread-safe, because the CPU list should be moved
> under RCU protection (due to the call to !all_cpu_threads_idle()
> in qemu_clock_warp).  However it is a substantial step forward, the
> only uncovered case being CPU hotplug.
> 
> Please review.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Not familiar with the icount code but overall the patches look good and
it comes down to placing a seqlock around icount.