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

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

[Liu Ping Fan]

v5:
  fine rename some variable in patch2&4. 
  fix commit log for patch1&2

v4:
  fix commit log for "protect timers_state's clock with seqlock"  (Thanks for Alex)

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                      |  41 +++++++++++++---
 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        |   8 +++
 qemu-timer.c                |  21 +++++++-
 util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
 util/qemu-thread-win32.c    |  26 ++++++++++
 9 files changed, 294 insertions(+), 9 deletions(-)
 create mode 100644 include/qemu/seqlock.h

-- 
1.8.1.4

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

[Liu Ping Fan]

From: Paolo Bonzini <pbonzini@redhat.com>

Seqlock implementation for QEMU. Usage idiom

reader:
  do{
    start = seqlock_read_begin()

  }while(seqlock_read_try(start))

writer:
  seqlock_write_lock()
  ...
  seqlock_write_unlock()

initialization:
  seqlock_init(QemuSeqLock *sl, QemuMutex *mutex)
  where mutex could be NULL if the caller will provide extra lock
  protection for seqlock_write_lock.

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

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

[Paolo Bonzini]

Il 25/09/2013 08:20, Liu Ping Fan ha scritto:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Seqlock implementation for QEMU. Usage idiom
> 
> reader:
>   do{
>     start = seqlock_read_begin()
> 
>   }while(seqlock_read_try(start))
> 
> writer:
>   seqlock_write_lock()
>   ...
>   seqlock_write_unlock()
> 
> initialization:
>   seqlock_init(QemuSeqLock *sl, QemuMutex *mutex)
>   where mutex could be NULL if the caller will provide extra lock
>   protection for seqlock_write_lock.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

You need to include your own signoff in addition to mine, here and in
patch 3.

Just reply to the two patches with the Signed-off-by line, whoever
applies can pick it up.

Paolo

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

[liu ping fan]

On Wed, Sep 25, 2013 at 2:20 PM, Liu Ping Fan <qemulist@gmail.com> wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> Seqlock implementation for QEMU. Usage idiom
>
> reader:
>   do{
>     start = seqlock_read_begin()
>
>   }while(seqlock_read_try(start))
>
> writer:
>   seqlock_write_lock()
>   ...
>   seqlock_write_unlock()
>
> initialization:
>   seqlock_init(QemuSeqLock *sl, QemuMutex *mutex)
>   where mutex could be NULL if the caller will provide extra lock
>   protection for seqlock_write_lock.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.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 v5 2/4] timer: protect timers_state's clock with seqlock

[Liu Ping Fan]

QEMU_CLOCK_VIRTUAL may be read outside BQL. This will make its
foundation, i.e. cpu_clock_offset exposed to race condition.
Using private lock to protect it.

After this patch, reading QEMU_CLOCK_VIRTUAL is thread safe
unless use_icount is true, in which case the existing callers
still rely on the BQL

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

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpus.c               | 41 +++++++++++++++++++++++++++++++++--------
 include/qemu/timer.h |  2 ++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/cpus.c b/cpus.c
index e566297..5baa76d 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,11 @@ 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 cpu_clock_offset_seqlock;
     int64_t cpu_clock_offset;
     int32_t cpu_ticks_enabled;
     int64_t dummy;
@@ -137,6 +143,7 @@ int64_t cpu_get_icount(void)
 }
 
 /* return the host CPU cycle counter and handle stop/restart */
+/* Caller must hold the BQL */
 int64_t cpu_get_ticks(void)
 {
     if (use_icount) {
@@ -161,33 +168,50 @@ 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.cpu_clock_offset_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.cpu_clock_offset_seqlock, start));
+
+    return ti;
 }
 
-/* enable cpu_get_ticks() */
+/* enable cpu_get_ticks()
+ * Caller must hold BQL which server as mutex for cpu_clock_offset_seqlock.
+ */
 void cpu_enable_ticks(void)
 {
+    /* Here, the really thing protected by seqlock is cpu_clock_offset. */
+    seqlock_write_lock(&timers_state.cpu_clock_offset_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.cpu_clock_offset_seqlock);
 }
 
 /* disable cpu_get_ticks() : the clock is stopped. You must not call
-   cpu_get_ticks() after that.  */
+ * cpu_get_ticks() after that.
+ * Caller must hold BQL which server as mutex for cpu_clock_offset_seqlock.
+ */
 void cpu_disable_ticks(void)
 {
+    /* Here, the really thing protected by seqlock is cpu_clock_offset. */
+    seqlock_write_lock(&timers_state.cpu_clock_offset_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.cpu_clock_offset_seqlock);
 }
 
 /* Correlation between real and virtual time is always going to be
@@ -371,6 +395,7 @@ static const VMStateDescription vmstate_timers = {
 
 void configure_icount(const char *option)
 {
+    seqlock_init(&timers_state.cpu_clock_offset_seqlock, NULL);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
     if (!option) {
         return;
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index e4934dd..bb1de23 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -636,7 +636,9 @@ static inline int64_t qemu_soonest_timeout(int64_t timeout1, int64_t timeout2)
 void init_clocks(void);
 
 int64_t cpu_get_ticks(void);
+/* Caller must hold BQL */
 void cpu_enable_ticks(void);
+/* Caller must hold BQL */
 void cpu_disable_ticks(void);
 
 static inline int64_t get_ticks_per_sec(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH v5 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 v5 3/4] qemu-thread: add QemuEvent

[liu ping fan]

On Wed, Sep 25, 2013 at 2:20 PM, Liu Ping Fan <qemulist@gmail.com> wrote:
> 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>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.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
>

[Qemu-devel] [PATCH v5 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 |  6 ++++++
 qemu-timer.c         | 21 ++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index bb1de23..c7fa83a 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -185,6 +185,12 @@ 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.
+ *
+ * Caller should hold BQL.
  */
 void qemu_clock_enable(QEMUClockType type, bool enabled);
 
diff --git a/qemu-timer.c b/qemu-timer.c
index 95ff47f..3a828aa 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 timers_done_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->timers_done_ev, false);
     timer_list->clock = clock;
     timer_list->notify_cb = cb;
     timer_list->notify_opaque = opaque;
@@ -140,13 +144,25 @@ 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.
+ *
+ * Caller should hold BQL.
+ */
 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) {
+        QLIST_FOREACH(tl, &clock->timerlists, list) {
+            qemu_event_wait(&tl->timers_done_ev);
+        }
     }
 }
 
@@ -373,8 +389,10 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     QEMUTimer *ts;
     int64_t current_time;
     bool progress = false;
-   
+
+    qemu_event_reset(&timer_list->timers_done_ev);
     if (!timer_list->clock->enabled) {
+        qemu_event_set(&timer_list->timers_done_ev);
         return progress;
     }
 
@@ -392,6 +410,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         ts->cb(ts->opaque);
         progress = true;
     }
+    qemu_event_set(&timer_list->timers_done_ev);
     return progress;
 }
 
-- 
1.8.1.4

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

[Paolo Bonzini]

Stefan, will you pick this up next week or shall I?

I have patches for thread-safe icount almost ready to post, and I am not
sure through whom they are going to go.

Paolo

Il 25/09/2013 08:20, Liu Ping Fan ha scritto:
> v5:
>   fine rename some variable in patch2&4. 
>   fix commit log for patch1&2
> 
> v4:
>   fix commit log for "protect timers_state's clock with seqlock"  (Thanks for Alex)
> 
> 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                      |  41 +++++++++++++---
>  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        |   8 +++
>  qemu-timer.c                |  21 +++++++-
>  util/qemu-thread-posix.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++
>  util/qemu-thread-win32.c    |  26 ++++++++++
>  9 files changed, 294 insertions(+), 9 deletions(-)
>  create mode 100644 include/qemu/seqlock.h
> 

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

[Stefan Hajnoczi]

On Mon, Oct 07, 2013 at 02:24:26PM +0200, Paolo Bonzini wrote:
> Stefan, will you pick this up next week or shall I?
> 
> I have patches for thread-safe icount almost ready to post, and I am not
> sure through whom they are going to go.

Please include it in your pull request.

Kevin is merging block patches this week, the queue is fairly full so I
imagine he has plenty of other things to review.

Stefan