[Qemu-devel] [PATCH v3 0/4] timers thread-safe stuff

[Qemu-devel] [PATCH v3 0/4] timers thread-safe stuff

[Liu Ping Fan]

Saw the Alex's patches has been merged, rebase mine onto his.

v3:
  1. rename seqlock_read_check as seqlock_read_retry
  2. Document timerlist were protected by BQL, and discard private lock around "qemu_event_wait(tl->ev)".

v2:
  1. fix comment in commit and code
  2. fix race issue for qemu_clock_enable(foo,disable)


Liu Ping Fan (2):
  timer: protect timers_state's clock with seqlock
  timer: make qemu_clock_enable sync between disable and timer's cb

Paolo Bonzini (2):
  seqlock: introduce read-write seqlock
  qemu-thread: add QemuEvent

 cpus.c                      |  36 +++++++++++---
 include/qemu/seqlock.h      |  72 +++++++++++++++++++++++++++
 include/qemu/thread-posix.h |   8 +++
 include/qemu/thread-win32.h |   4 ++
 include/qemu/thread.h       |   7 +++
 include/qemu/timer.h        |   4 ++
 qemu-timer.c                |  20 +++++++-
 util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
 util/qemu-thread-win32.c    |  26 ++++++++++
 9 files changed, 286 insertions(+), 7 deletions(-)
 create mode 100644 include/qemu/seqlock.h

-- 
1.8.1.4

[Qemu-devel] [PATCH v3 1/4] seqlock: introduce read-write seqlock

[Liu Ping Fan]

From: Paolo Bonzini <pbonzini@redhat.com>

This lets the read-side access run outside the BQL.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/seqlock.h | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 include/qemu/seqlock.h

diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
new file mode 100644
index 0000000..3ff118a
--- /dev/null
+++ b/include/qemu/seqlock.h
@@ -0,0 +1,72 @@
+/*
+ * Seqlock implementation for QEMU
+ *
+ * Copyright Red Hat, Inc. 2013
+ *
+ * Author:
+ *  Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef QEMU_SEQLOCK_H
+#define QEMU_SEQLOCK_H 1
+
+#include <qemu/atomic.h>
+#include <qemu/thread.h>
+
+typedef struct QemuSeqLock QemuSeqLock;
+
+struct QemuSeqLock {
+    QemuMutex *mutex;
+    unsigned sequence;
+};
+
+static inline void seqlock_init(QemuSeqLock *sl, QemuMutex *mutex)
+{
+    sl->mutex = mutex;
+    sl->sequence = 0;
+}
+
+/* Lock out other writers and update the count.  */
+static inline void seqlock_write_lock(QemuSeqLock *sl)
+{
+    if (sl->mutex) {
+        qemu_mutex_lock(sl->mutex);
+    }
+    ++sl->sequence;
+
+    /* Write sequence before updating other fields.  */
+    smp_wmb();
+}
+
+static inline void seqlock_write_unlock(QemuSeqLock *sl)
+{
+    /* Write other fields before finalizing sequence.  */
+    smp_wmb();
+
+    ++sl->sequence;
+    if (sl->mutex) {
+        qemu_mutex_unlock(sl->mutex);
+    }
+}
+
+static inline unsigned seqlock_read_begin(QemuSeqLock *sl)
+{
+    /* Always fail if a write is in progress.  */
+    unsigned ret = sl->sequence & ~1;
+
+    /* Read sequence before reading other fields.  */
+    smp_rmb();
+    return ret;
+}
+
+static int seqlock_read_retry(const QemuSeqLock *sl, unsigned start)
+{
+    /* Read other fields before reading final sequence.  */
+    smp_rmb();
+    return unlikely(sl->sequence != start);
+}
+
+#endif
-- 
1.8.1.4

[Qemu-devel] [PATCH v3 2/4] timer: protect timers_state's clock with seqlock

[Liu Ping Fan]

The vm_clock may be read outside BQL. This will make timers_state
--the foundation of vm_clock exposed to race condition.
Using private lock to protect it.

Note in tcg mode, vm_clock still read inside BQL, so icount is
left without private lock's protection. As for cpu_ticks_* in
timers_state, it is still protected by BQL.

Lock rule: private lock innermost, ie BQL->"this lock"

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpus.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index b9e5685..bcead3b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -37,6 +37,7 @@
 #include "sysemu/qtest.h"
 #include "qemu/main-loop.h"
 #include "qemu/bitmap.h"
+#include "qemu/seqlock.h"
 
 #ifndef _WIN32
 #include "qemu/compatfd.h"
@@ -112,6 +113,13 @@ static int64_t qemu_icount;
 typedef struct TimersState {
     int64_t cpu_ticks_prev;
     int64_t cpu_ticks_offset;
+    /* cpu_clock_offset will be read out of BQL, so protect it with private
+     * lock. As for cpu_ticks_*, no requirement to read it outside BQL yet.
+     * Lock rule: innermost
+     */
+    QemuSeqLock clock_seqlock;
+    /* mutex for seqlock */
+    QemuMutex mutex;
     int64_t cpu_clock_offset;
     int32_t cpu_ticks_enabled;
     int64_t dummy;
@@ -137,6 +145,7 @@ int64_t cpu_get_icount(void)
 }
 
 /* return the host CPU cycle counter and handle stop/restart */
+/* cpu_ticks is safely if holding BQL */
 int64_t cpu_get_ticks(void)
 {
     if (use_icount) {
@@ -161,33 +170,46 @@ int64_t cpu_get_ticks(void)
 int64_t cpu_get_clock(void)
 {
     int64_t ti;
-    if (!timers_state.cpu_ticks_enabled) {
-        return timers_state.cpu_clock_offset;
-    } else {
-        ti = get_clock();
-        return ti + timers_state.cpu_clock_offset;
-    }
+    unsigned start;
+
+    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;
+        }
+    } while (seqlock_read_retry(&timers_state.clock_seqlock, start));
+
+    return ti;
 }
 
 /* enable cpu_get_ticks() */
 void cpu_enable_ticks(void)
 {
+    /* Here, the really thing protected by seqlock is cpu_clock_offset. */
+    seqlock_write_lock(&timers_state.clock_seqlock);
     if (!timers_state.cpu_ticks_enabled) {
         timers_state.cpu_ticks_offset -= cpu_get_real_ticks();
         timers_state.cpu_clock_offset -= get_clock();
         timers_state.cpu_ticks_enabled = 1;
     }
+    seqlock_write_unlock(&timers_state.clock_seqlock);
 }
 
 /* disable cpu_get_ticks() : the clock is stopped. You must not call
    cpu_get_ticks() after that.  */
 void cpu_disable_ticks(void)
 {
+    /* Here, the really thing protected by seqlock is cpu_clock_offset. */
+    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_ticks_enabled = 0;
     }
+    seqlock_write_unlock(&timers_state.clock_seqlock);
 }
 
 /* Correlation between real and virtual time is always going to be
@@ -371,6 +393,8 @@ static const VMStateDescription vmstate_timers = {
 
 void configure_icount(const char *option)
 {
+    qemu_mutex_init(&timers_state.mutex);
+    seqlock_init(&timers_state.clock_seqlock, &timers_state.mutex);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
     if (!option) {
         return;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3 2/4] timer: protect timers_state's clock with seqlock

[Alex Bligh]

On 27 Aug 2013, at 04:21, Liu Ping Fan wrote:

> Note in tcg mode, vm_clock still read inside BQL, so icount is

Should refer to QEMU_CLOCK_VIRTUAL if after my patches

> left without private lock's protection. As for cpu_ticks_* in
> timers_state, it is still protected by BQL.

I *think* what you are saying here is that after this patch,
reading QEMU_CLOCK_VIRTUAL is threadsafe unless use_icount
is true, in which case it is not thread safe as existing
callers rely on the BQL.

The commit could be a bit more specific here, not least as
if I read it right, that will need fixing before
QEMU_CLOCK_VIRTUAL is used at all in other other threads.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v3 2/4] timer: protect timers_state's clock with seqlock

[liu ping fan]

On Tue, Aug 27, 2013 at 11:18 PM, Alex Bligh <alex@alex.org.uk> wrote:
>
> On 27 Aug 2013, at 04:21, Liu Ping Fan wrote:
>
>> Note in tcg mode, vm_clock still read inside BQL, so icount is
>
> Should refer to QEMU_CLOCK_VIRTUAL if after my patches
>
Will change the log.
>> left without private lock's protection. As for cpu_ticks_* in
>> timers_state, it is still protected by BQL.
>
> I *think* what you are saying here is that after this patch,
> reading QEMU_CLOCK_VIRTUAL is threadsafe unless use_icount
> is true, in which case it is not thread safe as existing
> callers rely on the BQL.
>
Yes.
> The commit could be a bit more specific here, not least as
> if I read it right, that will need fixing before
> QEMU_CLOCK_VIRTUAL is used at all in other other threads.
>
Yes, this patch should be ready, before we use timers on other threads.

Thx,
Pingfan
> --
> Alex Bligh
>
>
>
>

[Qemu-devel] [PATCH v3 3/4] qemu-thread: add QemuEvent

[Liu Ping Fan]

From: Paolo Bonzini <pbonzini@redhat.com>

This emulates Win32 manual-reset events using futexes or conditional
variables.  Typical ways to use them are with multi-producer,
single-consumer data structures, to test for a complex condition whose
elements come from different threads:

    for (;;) {
        qemu_event_reset(ev);
        ... test complex condition ...
        if (condition is true) {
            break;
        }
        qemu_event_wait(ev);
    }

Or more efficiently (but with some duplication):

    ... evaluate condition ...
    while (!condition) {
        qemu_event_reset(ev);
        ... evaluate condition ...
        if (!condition) {
            qemu_event_wait(ev);
            ... evaluate condition ...
        }
    }

QemuEvent provides a very fast userspace path in the common case when
no other thread is waiting, or the event is not changing state.  It
is used to report RCU quiescent states to the thread calling
synchronize_rcu (the latter being the single consumer), and to report
call_rcu invocations to the thread that receives them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/thread-posix.h |   8 +++
 include/qemu/thread-win32.h |   4 ++
 include/qemu/thread.h       |   7 +++
 util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
 util/qemu-thread-win32.c    |  26 ++++++++++
 5 files changed, 161 insertions(+)

diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index 361566a..eb5c7a1 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -21,6 +21,14 @@ struct QemuSemaphore {
 #endif
 };
 
+struct QemuEvent {
+#ifndef __linux__
+    pthread_mutex_t lock;
+    pthread_cond_t cond;
+#endif
+    unsigned value;
+};
+
 struct QemuThread {
     pthread_t thread;
 };
diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
index 13adb95..3d58081 100644
--- a/include/qemu/thread-win32.h
+++ b/include/qemu/thread-win32.h
@@ -17,6 +17,10 @@ struct QemuSemaphore {
     HANDLE sema;
 };
 
+struct QemuEvent {
+    HANDLE event;
+};
+
 typedef struct QemuThreadData QemuThreadData;
 struct QemuThread {
     QemuThreadData *data;
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index c02404b..3e32c65 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -7,6 +7,7 @@
 typedef struct QemuMutex QemuMutex;
 typedef struct QemuCond QemuCond;
 typedef struct QemuSemaphore QemuSemaphore;
+typedef struct QemuEvent QemuEvent;
 typedef struct QemuThread QemuThread;
 
 #ifdef _WIN32
@@ -45,6 +46,12 @@ void qemu_sem_wait(QemuSemaphore *sem);
 int qemu_sem_timedwait(QemuSemaphore *sem, int ms);
 void qemu_sem_destroy(QemuSemaphore *sem);
 
+void qemu_event_init(QemuEvent *ev, bool init);
+void qemu_event_set(QemuEvent *ev);
+void qemu_event_reset(QemuEvent *ev);
+void qemu_event_wait(QemuEvent *ev);
+void qemu_event_destroy(QemuEvent *ev);
+
 void qemu_thread_create(QemuThread *thread,
                         void *(*start_routine)(void *),
                         void *arg, int mode);
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 4de133e..37dd298 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -20,7 +20,12 @@
 #include <limits.h>
 #include <unistd.h>
 #include <sys/time.h>
+#ifdef __linux__
+#include <sys/syscall.h>
+#include <linux/futex.h>
+#endif
 #include "qemu/thread.h"
+#include "qemu/atomic.h"
 
 static void error_exit(int err, const char *msg)
 {
@@ -272,6 +277,117 @@ void qemu_sem_wait(QemuSemaphore *sem)
 #endif
 }
 
+#ifdef __linux__
+#define futex(...)              syscall(__NR_futex, __VA_ARGS__)
+
+static inline void futex_wake(QemuEvent *ev, int n)
+{
+    futex(ev, FUTEX_WAKE, n, NULL, NULL, 0);
+}
+
+static inline void futex_wait(QemuEvent *ev, unsigned val)
+{
+    futex(ev, FUTEX_WAIT, (int) val, NULL, NULL, 0);
+}
+#else
+static inline void futex_wake(QemuEvent *ev, int n)
+{
+    if (n == 1) {
+        pthread_cond_signal(&ev->cond);
+    } else {
+        pthread_cond_broadcast(&ev->cond);
+    }
+}
+
+static inline void futex_wait(QemuEvent *ev, unsigned val)
+{
+    pthread_mutex_lock(&ev->lock);
+    if (ev->value == val) {
+        pthread_cond_wait(&ev->cond, &ev->lock);
+    }
+    pthread_mutex_unlock(&ev->lock);
+}
+#endif
+
+/* Valid transitions:
+ * - free->set, when setting the event
+ * - busy->set, when setting the event, followed by futex_wake
+ * - set->free, when resetting the event
+ * - free->busy, when waiting
+ *
+ * set->busy does not happen (it can be observed from the outside but
+ * it really is set->free->busy).
+ *
+ * busy->free provably cannot happen; to enforce it, the set->free transition
+ * is done with an OR, which becomes a no-op if the event has concurrently
+ * transitioned to free or busy.
+ */
+
+#define EV_SET         0
+#define EV_FREE        1
+#define EV_BUSY       -1
+
+void qemu_event_init(QemuEvent *ev, bool init)
+{
+#ifndef __linux__
+    pthread_mutex_init(&ev->lock, NULL);
+    pthread_cond_init(&ev->cond, NULL);
+#endif
+
+    ev->value = (init ? EV_SET : EV_FREE);
+}
+
+void qemu_event_destroy(QemuEvent *ev)
+{
+#ifndef __linux__
+    pthread_mutex_destroy(&ev->lock);
+    pthread_cond_destroy(&ev->cond);
+#endif
+}
+
+void qemu_event_set(QemuEvent *ev)
+{
+    if (atomic_mb_read(&ev->value) != EV_SET) {
+        if (atomic_xchg(&ev->value, EV_SET) == EV_BUSY) {
+            /* There were waiters, wake them up.  */
+            futex_wake(ev, INT_MAX);
+        }
+    }
+}
+
+void qemu_event_reset(QemuEvent *ev)
+{
+    if (atomic_mb_read(&ev->value) == EV_SET) {
+        /*
+         * If there was a concurrent reset (or even reset+wait),
+         * do nothing.  Otherwise change EV_SET->EV_FREE.
+         */
+        atomic_or(&ev->value, EV_FREE);
+    }
+}
+
+void qemu_event_wait(QemuEvent *ev)
+{
+    unsigned value;
+
+    value = atomic_mb_read(&ev->value);
+    if (value != EV_SET) {
+        if (value == EV_FREE) {
+            /*
+             * Leave the event reset and tell qemu_event_set that there
+             * are waiters.  No need to retry, because there cannot be
+             * a concurent busy->free transition.  After the CAS, the
+             * event will be either set or busy.
+             */
+            if (atomic_cmpxchg(&ev->value, EV_FREE, EV_BUSY) == EV_SET) {
+                return;
+            }
+        }
+        futex_wait(ev, EV_BUSY);
+    }
+}
+
+
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 517878d..27a5217 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -227,6 +227,32 @@ void qemu_sem_wait(QemuSemaphore *sem)
     }
 }
 
+void qemu_event_init(QemuEvent *ev, bool init)
+{
+    /* Manual reset.  */
+    ev->event = CreateEvent(NULL, TRUE, init, NULL);
+}
+
+void qemu_event_destroy(QemuEvent *ev)
+{
+    CloseHandle(ev->event);
+}
+
+void qemu_event_set(QemuEvent *ev)
+{
+    SetEvent(ev->event);
+}
+
+void qemu_event_reset(QemuEvent *ev)
+{
+    ResetEvent(ev->event);
+}
+
+void qemu_event_wait(QemuEvent *ev)
+{
+    WaitForSingleObject(ev->event, INFINITE);
+}
+
 struct QemuThreadData {
     /* Passed to win32_start_routine.  */
     void             *(*start_routine)(void *);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3 3/4] qemu-thread: add QemuEvent

[Alex Bligh]

On 27 Aug 2013, at 04:21, Liu Ping Fan wrote:

> +void qemu_event_set(QemuEvent *ev)
> +{

It would be useful if these functions had inline
documentation.

-- 
Alex Bligh

[Qemu-devel] [PATCH v3 4/4] timer: make qemu_clock_enable sync between disable and timer's cb

[Liu Ping Fan]

After disabling the QemuClock, we should make sure that no QemuTimers
are still in flight. To implement that with light overhead, we resort
to QemuEvent. The caller of disabling will wait on QemuEvent of each
timerlist.

Note, qemu_clock_enable(foo,false) can _not_ be called from timer's cb.
And the callers of qemu_clock_enable() should be sync by themselves,
not protected by this patch.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/timer.h |  4 ++++
 qemu-timer.c         | 20 +++++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index e4934dd..b26909a 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -185,6 +185,10 @@ void qemu_clock_notify(QEMUClockType type);
  * @enabled: true to enable, false to disable
  *
  * Enable or disable a clock
+ * Disabling the clock will wait for related timerlists to stop
+ * executing qemu_run_timers.  Thus, this functions should not
+ * be used from the callback of a timer that is based on @clock.
+ * Doing so would cause a deadlock.
  */
 void qemu_clock_enable(QEMUClockType type, bool enabled);
 
diff --git a/qemu-timer.c b/qemu-timer.c
index 95ff47f..c500a76 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -45,6 +45,7 @@
 /* timers */
 
 typedef struct QEMUClock {
+     /* We rely on BQL to protect the timerlists */
     QLIST_HEAD(, QEMUTimerList) timerlists;
 
     NotifierList reset_notifiers;
@@ -70,6 +71,8 @@ struct QEMUTimerList {
     QLIST_ENTRY(QEMUTimerList) list;
     QEMUTimerListNotifyCB *notify_cb;
     void *notify_opaque;
+    /* light weight method to mark the end of timerlist's running */
+    QemuEvent ev;
 };
 
 /**
@@ -98,6 +101,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
     QEMUClock *clock = qemu_clock_ptr(type);
 
     timer_list = g_malloc0(sizeof(QEMUTimerList));
+    qemu_event_init(&timer_list->ev, false);
     timer_list->clock = clock;
     timer_list->notify_cb = cb;
     timer_list->notify_opaque = opaque;
@@ -140,13 +144,24 @@ void qemu_clock_notify(QEMUClockType type)
     }
 }
 
+/* Disabling the clock will wait for related timerlists to stop
+ * executing qemu_run_timers.  Thus, this functions should not
+ * be used from the callback of a timer that is based on @clock.
+ * Doing so would cause a deadlock.
+ */
 void qemu_clock_enable(QEMUClockType type, bool enabled)
 {
     QEMUClock *clock = qemu_clock_ptr(type);
+    QEMUTimerList *tl;
     bool old = clock->enabled;
     clock->enabled = enabled;
     if (enabled && !old) {
         qemu_clock_notify(type);
+    } else if (!enabled && old) {
+        /* We rely on BQL to protect the timerlists */
+        QLIST_FOREACH(tl, &clock->timerlists, list) {
+            qemu_event_wait(&tl->ev);
+        }
     }
 }
 
@@ -373,8 +388,10 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     QEMUTimer *ts;
     int64_t current_time;
     bool progress = false;
-   
+
+    qemu_event_reset(&timer_list->ev);
     if (!timer_list->clock->enabled) {
+        qemu_event_set(&timer_list->ev);
         return progress;
     }
 
@@ -392,6 +409,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         ts->cb(ts->opaque);
         progress = true;
     }
+    qemu_event_set(&timer_list->ev);
     return progress;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3 4/4] timer: make qemu_clock_enable sync between disable and timer's cb

[Alex Bligh]

On 27 Aug 2013, at 04:21, Liu Ping Fan wrote:

> After disabling the QemuClock, we should make sure that no QemuTimers
> are still in flight. To implement that with light overhead, we resort
> to QemuEvent. The caller of disabling will wait on QemuEvent of each
> timerlist.
> 
> Note, qemu_clock_enable(foo,false) can _not_ be called from timer's cb.
> And the callers of qemu_clock_enable() should be sync by themselves,
> not protected by this patch.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> include/qemu/timer.h |  4 ++++
> qemu-timer.c         | 20 +++++++++++++++++++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index e4934dd..b26909a 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -185,6 +185,10 @@ void qemu_clock_notify(QEMUClockType type);
>  * @enabled: true to enable, false to disable
>  *
>  * Enable or disable a clock
> + * Disabling the clock will wait for related timerlists to stop
> + * executing qemu_run_timers.  Thus, this functions should not
> + * be used from the callback of a timer that is based on @clock.
> + * Doing so would cause a deadlock.

Presumably they should also be called with the BQL held?

>  */
> void qemu_clock_enable(QEMUClockType type, bool enabled);
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 95ff47f..c500a76 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -45,6 +45,7 @@
> /* timers */
> 
> typedef struct QEMUClock {
> +     /* We rely on BQL to protect the timerlists */
>     QLIST_HEAD(, QEMUTimerList) timerlists;
> 
>     NotifierList reset_notifiers;
> @@ -70,6 +71,8 @@ struct QEMUTimerList {
>     QLIST_ENTRY(QEMUTimerList) list;
>     QEMUTimerListNotifyCB *notify_cb;
>     void *notify_opaque;
> +    /* light weight method to mark the end of timerlist's running */
> +    QemuEvent ev;
> };
> 
> /**
> @@ -98,6 +101,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
>     QEMUClock *clock = qemu_clock_ptr(type);
> 
>     timer_list = g_malloc0(sizeof(QEMUTimerList));
> +    qemu_event_init(&timer_list->ev, false);
>     timer_list->clock = clock;
>     timer_list->notify_cb = cb;
>     timer_list->notify_opaque = opaque;
> @@ -140,13 +144,24 @@ void qemu_clock_notify(QEMUClockType type)
>     }
> }
> 
> +/* Disabling the clock will wait for related timerlists to stop
> + * executing qemu_run_timers.  Thus, this functions should not
> + * be used from the callback of a timer that is based on @clock.
> + * Doing so would cause a deadlock.
> + */
> void qemu_clock_enable(QEMUClockType type, bool enabled)
> {
>     QEMUClock *clock = qemu_clock_ptr(type);
> +    QEMUTimerList *tl;
>     bool old = clock->enabled;
>     clock->enabled = enabled;
>     if (enabled && !old) {
>         qemu_clock_notify(type);
> +    } else if (!enabled && old) {
> +        /* We rely on BQL to protect the timerlists */
> +        QLIST_FOREACH(tl, &clock->timerlists, list) {
> +            qemu_event_wait(&tl->ev);
> +        }
>     }
> }
> 
> @@ -373,8 +388,10 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>     QEMUTimer *ts;
>     int64_t current_time;
>     bool progress = false;
> -   
> +
> +    qemu_event_reset(&timer_list->ev);
>     if (!timer_list->clock->enabled) {
> +        qemu_event_set(&timer_list->ev);
>         return progress;
>     }
> 
> @@ -392,6 +409,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>         ts->cb(ts->opaque);
>         progress = true;
>     }
> +    qemu_event_set(&timer_list->ev);
>     return progress;
> }
> 
> -- 
> 1.8.1.4
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v3 4/4] timer: make qemu_clock_enable sync between disable and timer's cb

[liu ping fan]

On Tue, Aug 27, 2013 at 11:32 PM, Alex Bligh <alex@alex.org.uk> wrote:
>
> On 27 Aug 2013, at 04:21, Liu Ping Fan wrote:
>
>> After disabling the QemuClock, we should make sure that no QemuTimers
>> are still in flight. To implement that with light overhead, we resort
>> to QemuEvent. The caller of disabling will wait on QemuEvent of each
>> timerlist.
>>
>> Note, qemu_clock_enable(foo,false) can _not_ be called from timer's cb.
>> And the callers of qemu_clock_enable() should be sync by themselves,
>> not protected by this patch.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>> include/qemu/timer.h |  4 ++++
>> qemu-timer.c         | 20 +++++++++++++++++++-
>> 2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
>> index e4934dd..b26909a 100644
>> --- a/include/qemu/timer.h
>> +++ b/include/qemu/timer.h
>> @@ -185,6 +185,10 @@ void qemu_clock_notify(QEMUClockType type);
>>  * @enabled: true to enable, false to disable
>>  *
>>  * Enable or disable a clock
>> + * Disabling the clock will wait for related timerlists to stop
>> + * executing qemu_run_timers.  Thus, this functions should not
>> + * be used from the callback of a timer that is based on @clock.
>> + * Doing so would cause a deadlock.
>
> Presumably they should also be called with the BQL held?
>
Yes, will document this.

Thx,
Pingfan
>>  */
>> void qemu_clock_enable(QEMUClockType type, bool enabled);
>>
>> diff --git a/qemu-timer.c b/qemu-timer.c
>> index 95ff47f..c500a76 100644
>> --- a/qemu-timer.c
>> +++ b/qemu-timer.c
>> @@ -45,6 +45,7 @@
>> /* timers */
>>
>> typedef struct QEMUClock {
>> +     /* We rely on BQL to protect the timerlists */
>>     QLIST_HEAD(, QEMUTimerList) timerlists;
>>
>>     NotifierList reset_notifiers;
>> @@ -70,6 +71,8 @@ struct QEMUTimerList {
>>     QLIST_ENTRY(QEMUTimerList) list;
>>     QEMUTimerListNotifyCB *notify_cb;
>>     void *notify_opaque;
>> +    /* light weight method to mark the end of timerlist's running */
>> +    QemuEvent ev;
>> };
>>
>> /**
>> @@ -98,6 +101,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
>>     QEMUClock *clock = qemu_clock_ptr(type);
>>
>>     timer_list = g_malloc0(sizeof(QEMUTimerList));
>> +    qemu_event_init(&timer_list->ev, false);
>>     timer_list->clock = clock;
>>     timer_list->notify_cb = cb;
>>     timer_list->notify_opaque = opaque;
>> @@ -140,13 +144,24 @@ void qemu_clock_notify(QEMUClockType type)
>>     }
>> }
>>
>> +/* Disabling the clock will wait for related timerlists to stop
>> + * executing qemu_run_timers.  Thus, this functions should not
>> + * be used from the callback of a timer that is based on @clock.
>> + * Doing so would cause a deadlock.
>> + */
>> void qemu_clock_enable(QEMUClockType type, bool enabled)
>> {
>>     QEMUClock *clock = qemu_clock_ptr(type);
>> +    QEMUTimerList *tl;
>>     bool old = clock->enabled;
>>     clock->enabled = enabled;
>>     if (enabled && !old) {
>>         qemu_clock_notify(type);
>> +    } else if (!enabled && old) {
>> +        /* We rely on BQL to protect the timerlists */
>> +        QLIST_FOREACH(tl, &clock->timerlists, list) {
>> +            qemu_event_wait(&tl->ev);
>> +        }
>>     }
>> }
>>
>> @@ -373,8 +388,10 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>>     QEMUTimer *ts;
>>     int64_t current_time;
>>     bool progress = false;
>> -
>> +
>> +    qemu_event_reset(&timer_list->ev);
>>     if (!timer_list->clock->enabled) {
>> +        qemu_event_set(&timer_list->ev);
>>         return progress;
>>     }
>>
>> @@ -392,6 +409,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>>         ts->cb(ts->opaque);
>>         progress = true;
>>     }
>> +    qemu_event_set(&timer_list->ev);
>>     return progress;
>> }
>>
>> --
>> 1.8.1.4
>>
>>
>>
>
> --
> Alex Bligh
>
>
>
>

Re: [Qemu-devel] [PATCH v3 0/4] timers thread-safe stuff

[Paolo Bonzini]

Il 27/08/2013 05:20, Liu Ping Fan ha scritto:
> Saw the Alex's patches has been merged, rebase mine onto his.
> 
> v3:
>   1. rename seqlock_read_check as seqlock_read_retry
>   2. Document timerlist were protected by BQL, and discard private lock around "qemu_event_wait(tl->ev)".
> 
> v2:
>   1. fix comment in commit and code
>   2. fix race issue for qemu_clock_enable(foo,disable)
> 
> 
> Liu Ping Fan (2):
>   timer: protect timers_state's clock with seqlock
>   timer: make qemu_clock_enable sync between disable and timer's cb
> 
> Paolo Bonzini (2):
>   seqlock: introduce read-write seqlock
>   qemu-thread: add QemuEvent
> 
>  cpus.c                      |  36 +++++++++++---
>  include/qemu/seqlock.h      |  72 +++++++++++++++++++++++++++
>  include/qemu/thread-posix.h |   8 +++
>  include/qemu/thread-win32.h |   4 ++
>  include/qemu/thread.h       |   7 +++
>  include/qemu/timer.h        |   4 ++
>  qemu-timer.c                |  20 +++++++-
>  util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
>  util/qemu-thread-win32.c    |  26 ++++++++++
>  9 files changed, 286 insertions(+), 7 deletions(-)
>  create mode 100644 include/qemu/seqlock.h
> 

Stefan, could you pick up these four patches as well?

Paolo

Re: [Qemu-devel] [PATCH v3 0/4] timers thread-safe stuff

[Stefan Hajnoczi]

On Thu, Sep 12, 2013 at 01:19:47PM +0200, Paolo Bonzini wrote:
> Il 27/08/2013 05:20, Liu Ping Fan ha scritto:
> > Saw the Alex's patches has been merged, rebase mine onto his.
> > 
> > v3:
> >   1. rename seqlock_read_check as seqlock_read_retry
> >   2. Document timerlist were protected by BQL, and discard private lock around "qemu_event_wait(tl->ev)".
> > 
> > v2:
> >   1. fix comment in commit and code
> >   2. fix race issue for qemu_clock_enable(foo,disable)
> > 
> > 
> > Liu Ping Fan (2):
> >   timer: protect timers_state's clock with seqlock
> >   timer: make qemu_clock_enable sync between disable and timer's cb
> > 
> > Paolo Bonzini (2):
> >   seqlock: introduce read-write seqlock
> >   qemu-thread: add QemuEvent
> > 
> >  cpus.c                      |  36 +++++++++++---
> >  include/qemu/seqlock.h      |  72 +++++++++++++++++++++++++++
> >  include/qemu/thread-posix.h |   8 +++
> >  include/qemu/thread-win32.h |   4 ++
> >  include/qemu/thread.h       |   7 +++
> >  include/qemu/timer.h        |   4 ++
> >  qemu-timer.c                |  20 +++++++-
> >  util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
> >  util/qemu-thread-win32.c    |  26 ++++++++++
> >  9 files changed, 286 insertions(+), 7 deletions(-)
> >  create mode 100644 include/qemu/seqlock.h
> > 
> 
> Stefan, could you pick up these four patches as well?

This week Kevin is merging patches into the block tree.  If it's still
on the list next week I'll pick these patches up.

Stefan

Re: [Qemu-devel] [PATCH v3 0/4] timers thread-safe stuff

[Stefan Hajnoczi]

On Tue, Aug 27, 2013 at 11:20:59AM +0800, Liu Ping Fan wrote:
> Saw the Alex's patches has been merged, rebase mine onto his.
> 
> v3:
>   1. rename seqlock_read_check as seqlock_read_retry
>   2. Document timerlist were protected by BQL, and discard private lock around "qemu_event_wait(tl->ev)".
> 
> v2:
>   1. fix comment in commit and code
>   2. fix race issue for qemu_clock_enable(foo,disable)
> 
> 
> Liu Ping Fan (2):
>   timer: protect timers_state's clock with seqlock
>   timer: make qemu_clock_enable sync between disable and timer's cb
> 
> Paolo Bonzini (2):
>   seqlock: introduce read-write seqlock
>   qemu-thread: add QemuEvent
> 
>  cpus.c                      |  36 +++++++++++---
>  include/qemu/seqlock.h      |  72 +++++++++++++++++++++++++++
>  include/qemu/thread-posix.h |   8 +++
>  include/qemu/thread-win32.h |   4 ++
>  include/qemu/thread.h       |   7 +++
>  include/qemu/timer.h        |   4 ++
>  qemu-timer.c                |  20 +++++++-
>  util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
>  util/qemu-thread-win32.c    |  26 ++++++++++
>  9 files changed, 286 insertions(+), 7 deletions(-)
>  create mode 100644 include/qemu/seqlock.h

Ping Fan: Can you send a final version that addresses Alex's request for
documentation?

Otherwise we're ready to go.

Stefan

Re: [Qemu-devel] [PATCH v3 0/4] timers thread-safe stuff

[liu ping fan]

On Wed, Sep 18, 2013 at 9:54 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Aug 27, 2013 at 11:20:59AM +0800, Liu Ping Fan wrote:
>> Saw the Alex's patches has been merged, rebase mine onto his.
>>
>> v3:
>>   1. rename seqlock_read_check as seqlock_read_retry
>>   2. Document timerlist were protected by BQL, and discard private lock around "qemu_event_wait(tl->ev)".
>>
>> v2:
>>   1. fix comment in commit and code
>>   2. fix race issue for qemu_clock_enable(foo,disable)
>>
>>
>> Liu Ping Fan (2):
>>   timer: protect timers_state's clock with seqlock
>>   timer: make qemu_clock_enable sync between disable and timer's cb
>>
>> Paolo Bonzini (2):
>>   seqlock: introduce read-write seqlock
>>   qemu-thread: add QemuEvent
>>
>>  cpus.c                      |  36 +++++++++++---
>>  include/qemu/seqlock.h      |  72 +++++++++++++++++++++++++++
>>  include/qemu/thread-posix.h |   8 +++
>>  include/qemu/thread-win32.h |   4 ++
>>  include/qemu/thread.h       |   7 +++
>>  include/qemu/timer.h        |   4 ++
>>  qemu-timer.c                |  20 +++++++-
>>  util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
>>  util/qemu-thread-win32.c    |  26 ++++++++++
>>  9 files changed, 286 insertions(+), 7 deletions(-)
>>  create mode 100644 include/qemu/seqlock.h
>
> Ping Fan: Can you send a final version that addresses Alex's request for
> documentation?
>
> Otherwise we're ready to go.
>
Sorry, miss this letter, will update it immediately.

Thx,
Pingfan