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

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

[Liu Ping Fan]

This series aim to fix the rest race issue when run timers on multi-thread. (qemu_mod_ns has been fixed by Stefan by separated patches)
Have rebased onto Alex's patches "[RFC] [PATCHv10 00/31] aio / timers: Add AioContext timers and use ppoll"

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

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 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..8f1c89f
--- /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_check(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 v2 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 a6d7833..a748608 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"
@@ -107,6 +108,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;
@@ -132,6 +140,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) {
@@ -156,33 +165,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_check(&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
@@ -366,6 +388,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

[Qemu-devel] [PATCH v2 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

[Qemu-devel] [PATCH v2 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         | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 829c005..2b755c9 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -184,6 +184,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 5b9a722..8b32e92 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -48,6 +48,12 @@ typedef struct QEMUClock {
     QLIST_HEAD(, QEMUTimerList) timerlists;
 
     NotifierList reset_notifiers;
+    /* While the reader holds this lock, it may block on events_list.
+     * So the modifier should be carefuly not to reset the event before
+     * holding this lock. Otherwise, deadlock.
+     */
+    QemuMutex events_list_lock;
+    GList *events_list;
     int64_t last;
 
     QEMUClockType type;
@@ -70,6 +76,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;
 };
 
 /**
@@ -90,6 +98,25 @@ static bool timer_expired_ns(QEMUTimer *timer_head, int64_t current_time)
     return timer_head && (timer_head->expire_time <= current_time);
 }
 
+static QemuEvent *qemu_clock_new_qemu_event(QEMUClock *clock)
+{
+    QemuEvent *ev = g_malloc(sizeof(QemuEvent));
+
+    qemu_event_init(ev, true);
+    qemu_mutex_lock(&clock->events_list_lock);
+    clock->events_list = g_list_append(clock->events_list, ev);
+    qemu_mutex_unlock(&clock->events_list_lock);
+    return ev;
+}
+
+static void qemu_clock_free_qemu_event(QEMUClock *clock, QemuEvent *ev)
+{
+    qemu_mutex_lock(&clock->events_list_lock);
+    clock->events_list = g_list_remove(clock->events_list, ev);
+    qemu_mutex_unlock(&clock->events_list_lock);
+    qemu_event_destroy(ev);
+}
+
 QEMUTimerList *timerlist_new(QEMUClockType type,
                              QEMUTimerListNotifyCB *cb,
                              void *opaque)
@@ -98,6 +125,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
     QEMUClock *clock = qemu_clock_ptr(type);
 
     timer_list = g_malloc0(sizeof(QEMUTimerList));
+    timer_list->ev = qemu_clock_new_qemu_event(clock);
     timer_list->clock = clock;
     timer_list->notify_cb = cb;
     timer_list->notify_opaque = opaque;
@@ -109,6 +137,7 @@ void timerlist_free(QEMUTimerList *timer_list)
 {
     assert(!timerlist_has_timers(timer_list));
     if (timer_list->clock) {
+        qemu_clock_free_qemu_event(timer_list->clock, timer_list->ev);
         QLIST_REMOVE(timer_list, list);
     }
     g_free(timer_list);
@@ -122,6 +151,7 @@ static void qemu_clock_init(QEMUClockType type)
     clock->enabled = true;
     clock->last = INT64_MIN;
     QLIST_INIT(&clock->timerlists);
+    qemu_mutex_init(&clock->events_list_lock);
     notifier_list_init(&clock->reset_notifiers);
     main_loop_tlg.tl[type] = timerlist_new(type, NULL, NULL);
 }
@@ -140,6 +170,17 @@ void qemu_clock_notify(QEMUClockType type)
     }
 }
 
+static void clock_event_wait(gpointer key, gpointer opaque)
+{
+    QemuEvent *ev = key;
+    qemu_event_wait(ev);
+}
+
+/* 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);
@@ -147,6 +188,13 @@ void qemu_clock_enable(QEMUClockType type, bool enabled)
     clock->enabled = enabled;
     if (enabled && !old) {
         qemu_clock_notify(type);
+    } else if (!enabled && old) {
+        /* We may block while holding @events_list_lock, but the modifier is
+         * guaranteed not to reset the event. So we can avoid deadlock.
+         */
+        qemu_mutex_lock(&clock->events_list_lock);
+        g_list_foreach(clock->events_list, clock_event_wait, NULL);
+        qemu_mutex_unlock(&clock->events_list_lock);
     }
 }
 
@@ -373,8 +421,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 +442,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 v2 1/4] seqlock: introduce read-write seqlock

[Jan Kiszka]

On 2013-08-13 07:43, Liu Ping Fan wrote:
> 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..8f1c89f
> --- /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_check(const QemuSeqLock *sl, unsigned start)
> +{
> +    /* Read other fields before reading final sequence.  */
> +    smp_rmb();
> +    return unlikely(sl->sequence != start);
> +}
> +
> +#endif
> 

As the usage pattern is

seqlock_read_begin()
do
	...
while (seqlock_read_check())

I would suggest to call the latter seqlock_read_retry(), just like the
kernel does.

Jan

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

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

[liu ping fan]

On Tue, Aug 13, 2013 at 4:26 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-08-13 07:43, Liu Ping Fan wrote:
>> 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..8f1c89f
>> --- /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_check(const QemuSeqLock *sl, unsigned start)
>> +{
>> +    /* Read other fields before reading final sequence.  */
>> +    smp_rmb();
>> +    return unlikely(sl->sequence != start);
>> +}
>> +
>> +#endif
>>
>
> As the usage pattern is
>
> seqlock_read_begin()
> do
>         ...
> while (seqlock_read_check())
>
> I would suggest to call the latter seqlock_read_retry(), just like the
> kernel does.
>
I think it contains the meaning of check-success-or-retry. So may
check be nicer?
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

On 2013-08-13 10:39, liu ping fan wrote:
> On Tue, Aug 13, 2013 at 4:26 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2013-08-13 07:43, Liu Ping Fan wrote:
>>> 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..8f1c89f
>>> --- /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_check(const QemuSeqLock *sl, unsigned start)
>>> +{
>>> +    /* Read other fields before reading final sequence.  */
>>> +    smp_rmb();
>>> +    return unlikely(sl->sequence != start);
>>> +}
>>> +
>>> +#endif
>>>
>>
>> As the usage pattern is
>>
>> seqlock_read_begin()
>> do
>>         ...
>> while (seqlock_read_check())
>>
>> I would suggest to call the latter seqlock_read_retry(), just like the
>> kernel does.
>>
> I think it contains the meaning of check-success-or-retry. So may
> check be nicer?

"Check" alone has no obvious semantic for me, therefore "retry".

Jan

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

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

[Paolo Bonzini]

Il 13/08/2013 11:09, Jan Kiszka ha scritto:
>>>> >>>
>>> >>
>>> >> As the usage pattern is
>>> >>
>>> >> seqlock_read_begin()
>>> >> do
>>> >>         ...
>>> >> while (seqlock_read_check())
>>> >>
>>> >> I would suggest to call the latter seqlock_read_retry(), just like the
>>> >> kernel does.
>>> >>
>> > I think it contains the meaning of check-success-or-retry. So may
>> > check be nicer?
> "Check" alone has no obvious semantic for me, therefore "retry".

I agree.  I was writing the patch without looking at any Linux code
(just to be safe since it's GPLv2-only) and misremembered the name of
the API.

Paolo

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

[Paolo Bonzini]

Il 13/08/2013 07:43, Liu Ping Fan ha scritto:
> 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         | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 829c005..2b755c9 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -184,6 +184,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 5b9a722..8b32e92 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -48,6 +48,12 @@ typedef struct QEMUClock {
>      QLIST_HEAD(, QEMUTimerList) timerlists;
>  
>      NotifierList reset_notifiers;
> +    /* While the reader holds this lock, it may block on events_list.
> +     * So the modifier should be carefuly not to reset the event before
> +     * holding this lock. Otherwise, deadlock.
> +     */
> +    QemuMutex events_list_lock;
> +    GList *events_list;

No need for a separate list.  Just use the timerlists list; if
events_list needs a lock, timerlists needs one too.

>      int64_t last;
>  
>      QEMUClockType type;
> @@ -70,6 +76,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;

Also no need to malloc this one.

Paolo

>  };
>  
>  /**
> @@ -90,6 +98,25 @@ static bool timer_expired_ns(QEMUTimer *timer_head, int64_t current_time)
>      return timer_head && (timer_head->expire_time <= current_time);
>  }
>  
> +static QemuEvent *qemu_clock_new_qemu_event(QEMUClock *clock)
> +{
> +    QemuEvent *ev = g_malloc(sizeof(QemuEvent));
> +
> +    qemu_event_init(ev, true);
> +    qemu_mutex_lock(&clock->events_list_lock);
> +    clock->events_list = g_list_append(clock->events_list, ev);
> +    qemu_mutex_unlock(&clock->events_list_lock);
> +    return ev;
> +}
> +
> +static void qemu_clock_free_qemu_event(QEMUClock *clock, QemuEvent *ev)
> +{
> +    qemu_mutex_lock(&clock->events_list_lock);
> +    clock->events_list = g_list_remove(clock->events_list, ev);
> +    qemu_mutex_unlock(&clock->events_list_lock);
> +    qemu_event_destroy(ev);
> +}
> +
>  QEMUTimerList *timerlist_new(QEMUClockType type,
>                               QEMUTimerListNotifyCB *cb,
>                               void *opaque)
> @@ -98,6 +125,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
>      QEMUClock *clock = qemu_clock_ptr(type);
>  
>      timer_list = g_malloc0(sizeof(QEMUTimerList));
> +    timer_list->ev = qemu_clock_new_qemu_event(clock);
>      timer_list->clock = clock;
>      timer_list->notify_cb = cb;
>      timer_list->notify_opaque = opaque;
> @@ -109,6 +137,7 @@ void timerlist_free(QEMUTimerList *timer_list)
>  {
>      assert(!timerlist_has_timers(timer_list));
>      if (timer_list->clock) {
> +        qemu_clock_free_qemu_event(timer_list->clock, timer_list->ev);
>          QLIST_REMOVE(timer_list, list);
>      }
>      g_free(timer_list);
> @@ -122,6 +151,7 @@ static void qemu_clock_init(QEMUClockType type)
>      clock->enabled = true;
>      clock->last = INT64_MIN;
>      QLIST_INIT(&clock->timerlists);
> +    qemu_mutex_init(&clock->events_list_lock);
>      notifier_list_init(&clock->reset_notifiers);
>      main_loop_tlg.tl[type] = timerlist_new(type, NULL, NULL);
>  }
> @@ -140,6 +170,17 @@ void qemu_clock_notify(QEMUClockType type)
>      }
>  }
>  
> +static void clock_event_wait(gpointer key, gpointer opaque)
> +{
> +    QemuEvent *ev = key;
> +    qemu_event_wait(ev);
> +}
> +
> +/* 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);
> @@ -147,6 +188,13 @@ void qemu_clock_enable(QEMUClockType type, bool enabled)
>      clock->enabled = enabled;
>      if (enabled && !old) {
>          qemu_clock_notify(type);
> +    } else if (!enabled && old) {
> +        /* We may block while holding @events_list_lock, but the modifier is
> +         * guaranteed not to reset the event. So we can avoid deadlock.
> +         */
> +        qemu_mutex_lock(&clock->events_list_lock);
> +        g_list_foreach(clock->events_list, clock_event_wait, NULL);
> +        qemu_mutex_unlock(&clock->events_list_lock);
>      }
>  }
>  
> @@ -373,8 +421,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 +442,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>          ts->cb(ts->opaque);
>          progress = true;
>      }
> +    qemu_event_set(timer_list->ev);
>      return progress;
>  }
>  
> 

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

[Jan Kiszka]

On 2013-08-13 16:52, Paolo Bonzini wrote:
> Il 13/08/2013 11:09, Jan Kiszka ha scritto:
>>>>>>>>
>>>>>>
>>>>>> As the usage pattern is
>>>>>>
>>>>>> seqlock_read_begin()
>>>>>> do
>>>>>>         ...
>>>>>> while (seqlock_read_check())
>>>>>>
>>>>>> I would suggest to call the latter seqlock_read_retry(), just like the
>>>>>> kernel does.
>>>>>>
>>>> I think it contains the meaning of check-success-or-retry. So may
>>>> check be nicer?
>> "Check" alone has no obvious semantic for me, therefore "retry".
> 
> I agree.  I was writing the patch without looking at any Linux code
> (just to be safe since it's GPLv2-only) and misremembered the name of
> the API.

Another reason to forget about v2+. ;)

Jan

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

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

[Paolo Bonzini]

Il 13/08/2013 16:53, Jan Kiszka ha scritto:
>> > I agree.  I was writing the patch without looking at any Linux code
>> > (just to be safe since it's GPLv2-only) and misremembered the name of
>> > the API.
> Another reason to forget about v2+. ;)

Anyway, I am fairly sure this code is not based on Linux code, I never
looked at it.

Copying the API name from Linux does not "taint" the code with v2-onliness.

Paolo

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

[liu ping fan]

On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Il 13/08/2013 07:43, Liu Ping Fan ha scritto:
> > 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         | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> > index 829c005..2b755c9 100644
> > --- a/include/qemu/timer.h
> > +++ b/include/qemu/timer.h
> > @@ -184,6 +184,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 5b9a722..8b32e92 100644
> > --- a/qemu-timer.c
> > +++ b/qemu-timer.c
> > @@ -48,6 +48,12 @@ typedef struct QEMUClock {
> >      QLIST_HEAD(, QEMUTimerList) timerlists;
> >
> >      NotifierList reset_notifiers;
> > +    /* While the reader holds this lock, it may block on events_list.
> > +     * So the modifier should be carefuly not to reset the event before
> > +     * holding this lock. Otherwise, deadlock.
> > +     */
> > +    QemuMutex events_list_lock;
> > +    GList *events_list;
>
> No need for a separate list.  Just use the timerlists list; if
> events_list needs a lock, timerlists needs one too.
>
Here is a ugly pattern issue, we hold events_list_lock and wait for
QemuEvent set. If the modifier reset the QemuEvent and then try to
hold the events_list_lock, then _deadlock_.  To eliminate the
possibility, using  @events_list_lock, and you can see the modifier
can not reset QemuEvent while trying to own the lock. On the other
handle, if using lock on timerlists, since many entrance to access the
lock, we are not sure of avoiding deadlock

Regards,
Pingfan
> >      int64_t last;
> >
> >      QEMUClockType type;
> > @@ -70,6 +76,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;
>
> Also no need to malloc this one.
>
> Paolo
>
> >  };
> >
> >  /**
> > @@ -90,6 +98,25 @@ static bool timer_expired_ns(QEMUTimer *timer_head, int64_t current_time)
> >      return timer_head && (timer_head->expire_time <= current_time);
> >  }
> >
> > +static QemuEvent *qemu_clock_new_qemu_event(QEMUClock *clock)
> > +{
> > +    QemuEvent *ev = g_malloc(sizeof(QemuEvent));
> > +
> > +    qemu_event_init(ev, true);
> > +    qemu_mutex_lock(&clock->events_list_lock);
> > +    clock->events_list = g_list_append(clock->events_list, ev);
> > +    qemu_mutex_unlock(&clock->events_list_lock);
> > +    return ev;
> > +}
> > +
> > +static void qemu_clock_free_qemu_event(QEMUClock *clock, QemuEvent *ev)
> > +{
> > +    qemu_mutex_lock(&clock->events_list_lock);
> > +    clock->events_list = g_list_remove(clock->events_list, ev);
> > +    qemu_mutex_unlock(&clock->events_list_lock);
> > +    qemu_event_destroy(ev);
> > +}
> > +
> >  QEMUTimerList *timerlist_new(QEMUClockType type,
> >                               QEMUTimerListNotifyCB *cb,
> >                               void *opaque)
> > @@ -98,6 +125,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
> >      QEMUClock *clock = qemu_clock_ptr(type);
> >
> >      timer_list = g_malloc0(sizeof(QEMUTimerList));
> > +    timer_list->ev = qemu_clock_new_qemu_event(clock);
> >      timer_list->clock = clock;
> >      timer_list->notify_cb = cb;
> >      timer_list->notify_opaque = opaque;
> > @@ -109,6 +137,7 @@ void timerlist_free(QEMUTimerList *timer_list)
> >  {
> >      assert(!timerlist_has_timers(timer_list));
> >      if (timer_list->clock) {
> > +        qemu_clock_free_qemu_event(timer_list->clock, timer_list->ev);
> >          QLIST_REMOVE(timer_list, list);
> >      }
> >      g_free(timer_list);
> > @@ -122,6 +151,7 @@ static void qemu_clock_init(QEMUClockType type)
> >      clock->enabled = true;
> >      clock->last = INT64_MIN;
> >      QLIST_INIT(&clock->timerlists);
> > +    qemu_mutex_init(&clock->events_list_lock);
> >      notifier_list_init(&clock->reset_notifiers);
> >      main_loop_tlg.tl[type] = timerlist_new(type, NULL, NULL);
> >  }
> > @@ -140,6 +170,17 @@ void qemu_clock_notify(QEMUClockType type)
> >      }
> >  }
> >
> > +static void clock_event_wait(gpointer key, gpointer opaque)
> > +{
> > +    QemuEvent *ev = key;
> > +    qemu_event_wait(ev);
> > +}
> > +
> > +/* 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);
> > @@ -147,6 +188,13 @@ void qemu_clock_enable(QEMUClockType type, bool enabled)
> >      clock->enabled = enabled;
> >      if (enabled && !old) {
> >          qemu_clock_notify(type);
> > +    } else if (!enabled && old) {
> > +        /* We may block while holding @events_list_lock, but the modifier is
> > +         * guaranteed not to reset the event. So we can avoid deadlock.
> > +         */
> > +        qemu_mutex_lock(&clock->events_list_lock);
> > +        g_list_foreach(clock->events_list, clock_event_wait, NULL);
> > +        qemu_mutex_unlock(&clock->events_list_lock);
> >      }
> >  }
> >
> > @@ -373,8 +421,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 +442,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
> >          ts->cb(ts->opaque);
> >          progress = true;
> >      }
> > +    qemu_event_set(timer_list->ev);
> >      return progress;
> >  }
> >
> >
>

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

[Paolo Bonzini]

Il 14/08/2013 02:34, liu ping fan ha scritto:
> On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Il 13/08/2013 07:43, Liu Ping Fan ha scritto:
>>> 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         | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
>>> index 829c005..2b755c9 100644
>>> --- a/include/qemu/timer.h
>>> +++ b/include/qemu/timer.h
>>> @@ -184,6 +184,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 5b9a722..8b32e92 100644
>>> --- a/qemu-timer.c
>>> +++ b/qemu-timer.c
>>> @@ -48,6 +48,12 @@ typedef struct QEMUClock {
>>>      QLIST_HEAD(, QEMUTimerList) timerlists;
>>>
>>>      NotifierList reset_notifiers;
>>> +    /* While the reader holds this lock, it may block on events_list.
>>> +     * So the modifier should be carefuly not to reset the event before
>>> +     * holding this lock. Otherwise, deadlock.
>>> +     */
>>> +    QemuMutex events_list_lock;
>>> +    GList *events_list;
>>
>> No need for a separate list.  Just use the timerlists list; if
>> events_list needs a lock, timerlists needs one too.
>>
> Here is a ugly pattern issue, we hold events_list_lock and wait for
> QemuEvent set. If the modifier reset the QemuEvent and then try to
> hold the events_list_lock, then _deadlock_.  To eliminate the
> possibility, using  @events_list_lock, and you can see the modifier
> can not reset QemuEvent while trying to own the lock. On the other
> handle, if using lock on timerlists, since many entrance to access the
> lock, we are not sure of avoiding deadlock

But does timerlists need a lock, or does the BQL suffice?  If it
doesn't, there is no need for events_list_lock either.  Is
qemu_clock_enable called outside the BQL?

Paolo

> Regards,
> Pingfan
>>>      int64_t last;
>>>
>>>      QEMUClockType type;
>>> @@ -70,6 +76,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;
>>
>> Also no need to malloc this one.
>>
>> Paolo
>>
>>>  };
>>>
>>>  /**
>>> @@ -90,6 +98,25 @@ static bool timer_expired_ns(QEMUTimer *timer_head, int64_t current_time)
>>>      return timer_head && (timer_head->expire_time <= current_time);
>>>  }
>>>
>>> +static QemuEvent *qemu_clock_new_qemu_event(QEMUClock *clock)
>>> +{
>>> +    QemuEvent *ev = g_malloc(sizeof(QemuEvent));
>>> +
>>> +    qemu_event_init(ev, true);
>>> +    qemu_mutex_lock(&clock->events_list_lock);
>>> +    clock->events_list = g_list_append(clock->events_list, ev);
>>> +    qemu_mutex_unlock(&clock->events_list_lock);
>>> +    return ev;
>>> +}
>>> +
>>> +static void qemu_clock_free_qemu_event(QEMUClock *clock, QemuEvent *ev)
>>> +{
>>> +    qemu_mutex_lock(&clock->events_list_lock);
>>> +    clock->events_list = g_list_remove(clock->events_list, ev);
>>> +    qemu_mutex_unlock(&clock->events_list_lock);
>>> +    qemu_event_destroy(ev);
>>> +}
>>> +
>>>  QEMUTimerList *timerlist_new(QEMUClockType type,
>>>                               QEMUTimerListNotifyCB *cb,
>>>                               void *opaque)
>>> @@ -98,6 +125,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
>>>      QEMUClock *clock = qemu_clock_ptr(type);
>>>
>>>      timer_list = g_malloc0(sizeof(QEMUTimerList));
>>> +    timer_list->ev = qemu_clock_new_qemu_event(clock);
>>>      timer_list->clock = clock;
>>>      timer_list->notify_cb = cb;
>>>      timer_list->notify_opaque = opaque;
>>> @@ -109,6 +137,7 @@ void timerlist_free(QEMUTimerList *timer_list)
>>>  {
>>>      assert(!timerlist_has_timers(timer_list));
>>>      if (timer_list->clock) {
>>> +        qemu_clock_free_qemu_event(timer_list->clock, timer_list->ev);
>>>          QLIST_REMOVE(timer_list, list);
>>>      }
>>>      g_free(timer_list);
>>> @@ -122,6 +151,7 @@ static void qemu_clock_init(QEMUClockType type)
>>>      clock->enabled = true;
>>>      clock->last = INT64_MIN;
>>>      QLIST_INIT(&clock->timerlists);
>>> +    qemu_mutex_init(&clock->events_list_lock);
>>>      notifier_list_init(&clock->reset_notifiers);
>>>      main_loop_tlg.tl[type] = timerlist_new(type, NULL, NULL);
>>>  }
>>> @@ -140,6 +170,17 @@ void qemu_clock_notify(QEMUClockType type)
>>>      }
>>>  }
>>>
>>> +static void clock_event_wait(gpointer key, gpointer opaque)
>>> +{
>>> +    QemuEvent *ev = key;
>>> +    qemu_event_wait(ev);
>>> +}
>>> +
>>> +/* 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);
>>> @@ -147,6 +188,13 @@ void qemu_clock_enable(QEMUClockType type, bool enabled)
>>>      clock->enabled = enabled;
>>>      if (enabled && !old) {
>>>          qemu_clock_notify(type);
>>> +    } else if (!enabled && old) {
>>> +        /* We may block while holding @events_list_lock, but the modifier is
>>> +         * guaranteed not to reset the event. So we can avoid deadlock.
>>> +         */
>>> +        qemu_mutex_lock(&clock->events_list_lock);
>>> +        g_list_foreach(clock->events_list, clock_event_wait, NULL);
>>> +        qemu_mutex_unlock(&clock->events_list_lock);
>>>      }
>>>  }
>>>
>>> @@ -373,8 +421,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 +442,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>>>          ts->cb(ts->opaque);
>>>          progress = true;
>>>      }
>>> +    qemu_event_set(timer_list->ev);
>>>      return progress;
>>>  }
>>>
>>>
>>

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

[liu ping fan]

On Sun, Aug 18, 2013 at 10:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 14/08/2013 02:34, liu ping fan ha scritto:
>> On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> Il 13/08/2013 07:43, Liu Ping Fan ha scritto:
>>>> 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         | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
>>>> index 829c005..2b755c9 100644
>>>> --- a/include/qemu/timer.h
>>>> +++ b/include/qemu/timer.h
>>>> @@ -184,6 +184,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 5b9a722..8b32e92 100644
>>>> --- a/qemu-timer.c
>>>> +++ b/qemu-timer.c
>>>> @@ -48,6 +48,12 @@ typedef struct QEMUClock {
>>>>      QLIST_HEAD(, QEMUTimerList) timerlists;
>>>>
>>>>      NotifierList reset_notifiers;
>>>> +    /* While the reader holds this lock, it may block on events_list.
>>>> +     * So the modifier should be carefuly not to reset the event before
>>>> +     * holding this lock. Otherwise, deadlock.
>>>> +     */
>>>> +    QemuMutex events_list_lock;
>>>> +    GList *events_list;
>>>
>>> No need for a separate list.  Just use the timerlists list; if
>>> events_list needs a lock, timerlists needs one too.
>>>
>> Here is a ugly pattern issue, we hold events_list_lock and wait for
>> QemuEvent set. If the modifier reset the QemuEvent and then try to
>> hold the events_list_lock, then _deadlock_.  To eliminate the
>> possibility, using  @events_list_lock, and you can see the modifier
>> can not reset QemuEvent while trying to own the lock. On the other
>> handle, if using lock on timerlists, since many entrance to access the
>> lock, we are not sure of avoiding deadlock
>
> But does timerlists need a lock, or does the BQL suffice?  If it
> doesn't, there is no need for events_list_lock either.  Is
> qemu_clock_enable called outside the BQL?
>
Currently, no such guarantee based on BQL. But if we enforce that an
backend thread holds BQL before it cleans up resources, we can resort
to BQL. (so document this, and save the events_list_lock?)

Regards,
Pingfan

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

[Paolo Bonzini]

Il 19/08/2013 09:14, liu ping fan ha scritto:
>> > But does timerlists need a lock, or does the BQL suffice?  If it
>> > doesn't, there is no need for events_list_lock either.  Is
>> > qemu_clock_enable called outside the BQL?
>> >
> Currently, no such guarantee based on BQL. But if we enforce that an
> backend thread holds BQL before it cleans up resources, we can resort
> to BQL. (so document this, and save the events_list_lock?)

Yes, please.

Paolo