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

2013-09-24 Thread liu ping fan 
On Mon, Sep 23, 2013 at 2:21 PM, Jan Kiszka jan.kis...@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 pingf...@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

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

2013-09-23 Thread 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 pingf...@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

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

2013-09-22 Thread 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 pingf...@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