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

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

[Liu Ping Fan]

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                      |  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 v4 1/4] seqlock: introduce read-write seqlock

[Liu Ping Fan]

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

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

[Jan Kiszka]

On 2013-09-22 10:11, Liu Ping Fan wrote:
> This lets the read-side access run outside the BQL.

In fact, not only BQL. Didn't the original commit provide a changlog
about the content of this patch? Otherwise, briefly describe use cases
and maybe the typical invocation pattern.

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

>From says you, signed-off only Paolo - this is inconsistent.

Jan

> ---
>  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
> 

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

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

[liu ping fan]

On Mon, Sep 23, 2013 at 2:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-09-22 10:11, Liu Ping Fan wrote:
>> This lets the read-side access run outside the BQL.
>
> In fact, not only BQL. Didn't the original commit provide a changlog
> about the content of this patch? Otherwise, briefly describe use cases
> and maybe the typical invocation pattern.
>
Original commit provide no changelog (right? Paolo, if I do miss the
latest one in your tree).
What about the commit log like:

Seqlock implementation for QEMU. Usage idiom
reader:
seqlock_read_begin()
do{
}while(seqlock_read_try())

writer:
seqlock_write_lock()
...
seqlock_write_unlock()

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

>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> From says you, signed-off only Paolo - this is inconsistent.
>
Oh, a mistake, the author is Paolo!

Thanks and regards,
Pingfan
> Jan
>
>> ---
>>  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
>>
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux

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

[Paolo Bonzini]

Il 24/09/2013 07:33, liu ping fan ha scritto:
> On Mon, Sep 23, 2013 at 2:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2013-09-22 10:11, Liu Ping Fan wrote:
>>> This lets the read-side access run outside the BQL.
>>
>> In fact, not only BQL. Didn't the original commit provide a changlog
>> about the content of this patch? Otherwise, briefly describe use cases
>> and maybe the typical invocation pattern.
>>
> Original commit provide no changelog (right? Paolo, if I do miss the
> latest one in your tree).

Indeed I had never written one.

> What about the commit log like:
> 
> Seqlock implementation for QEMU. Usage idiom
> reader:
> seqlock_read_begin()
> do{
> }while(seqlock_read_try())
> 
> writer:
> seqlock_write_lock()
> ...
> seqlock_write_unlock()
> 
> initialization:
> seqlock_init(QemuSeqLock *sl, QemuMutex *mutex)
> where mutex could be NULL if the caller has provided extra lock
> protection for seqlock_write_lock.

replace "has provided" with "will provide".  Otherwise looks good.

Paolo

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

[Liu Ping Fan]

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 v4 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. timers_state 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 | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index e566297..870a832 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 v4 2/4] timer: protect timers_state's clock with seqlock

[Jan Kiszka]

On 2013-09-22 10:11, Liu Ping Fan wrote:
> QEMU_CLOCK_VIRTUAL may be read outside BQL. This will make its
> foundation, i.e. timers_state 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 | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index e566297..870a832 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;

If these locks only protect cpu_clock_offset, name them accordingly
(cpu_clock_offset_seqlock, cpu_clock_offset_mutex). But I think they
also protect cpu_ticks_enabled, no? Then you should adjust the comment.

>      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 */

"Caller must hold the 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;
> 

Jan

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

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

[liu ping fan]

On Mon, Sep 23, 2013 at 2:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-09-22 10:11, Liu Ping Fan wrote:
>> QEMU_CLOCK_VIRTUAL may be read outside BQL. This will make its
>> foundation, i.e. timers_state 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 | 36 ++++++++++++++++++++++++++++++------
>>  1 file changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index e566297..870a832 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;
>
> If these locks only protect cpu_clock_offset, name them accordingly
> (cpu_clock_offset_seqlock, cpu_clock_offset_mutex). But I think they

The mutex is internal for seqlock, not exported outside. So I think
the name is fine. But, I think we can drop it since the
cpu_enable_ticks/cpu_disable_ticks are always inside BQL.  So I will
rename clock_seqlock as cpu_clock_offset.
> also protect cpu_ticks_enabled, no? Then you should adjust the comment.
>
No. cpu_get_tsc()->cpu_get_ticks() is one reader of cpu_ticks_enabled,
which is protected against writers by BQL, not by this lock.

Thanks and regards,
Pingfan
>>      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 */
>
> "Caller must hold the 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;
>>
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux

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

[Liu Ping Fan]

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 v4 3/4] qemu-thread: add QemuEvent

[Jan Kiszka]

On 2013-09-22 10:11, Liu Ping Fan wrote:
> 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>

Again, from and signed-off mismatch.

BTW, above is a good template for a commit log of patch 1.

Jan

> ---
>  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 *);
> 

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

[Qemu-devel] [PATCH v4 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 v4 4/4] timer: make qemu_clock_enable sync between disable and timer's cb

[Jan Kiszka]

On 2013-09-22 10:11, 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.
>   */
>  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;

What about "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->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 */

So the caller of qemu_clock_enable has to hold the BQL? Then please add
that to the function description above instead.

Jan

> +        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;
>  }
>  
> 


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

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

[liu ping fan]

On Mon, Sep 23, 2013 at 2:26 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-09-22 10:11, 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.
>>   */
>>  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;
>
> What about "timers_done_ev"?
>
fine, thx.
>>  };
>>
>>  /**
>> @@ -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 */
>
> So the caller of qemu_clock_enable has to hold the BQL? Then please add
> that to the function description above instead.
>
Ok, thx.

Regards,
Pingfan
>> +        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;
>>  }
>>
>>
>
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux