Re: [Qemu-devel] [PATCH 2/5] cpus: use atomic_read to read seqlock-protected variables

[Paolo Bonzini]

> Is tsan happy with the way seqlocks are written right now?

I honestly don't know.  But if there are tsan bugs there's
not much we can do.  The alternative below has overhead on
ARM and PPC and does not quite fit in atomic.h.

In any case, a bigger issue is that this patch breaks on
32-bit because it does 64-bit atomic_read.  We might have
to fall back to volatile when not running on tsan.

Paolo

> Dmitry Vyukov wrote:
> > 1. Tsan is bad at handling stand-alone memory barriers.
> > And here is a way to express seqlock that is both correct, is
> > understood by tsan and is no overhead on x86:
> > 
> > // writer
> > atomic_store(, seq+1, memory_order_relaxed);
> > atomic_store([0], ..., memory_order_release);
> > ...
> > atomic_store([N], ..., memory_order_release);
> > atomic_store(, seq+1, memory_order_release);
> > 
> > // reader
> > atomic_load(, memory_order_acquire);
> > d0 = atomic_load([0], memory_order_acquire);
> > ...
> > dN = atomic_load([N], memory_order_acquire);
> > atomic_load(, memory_order_relaxed);
> 
> Source: https://groups.google.com/forum/#!topic/thread-sanitizer/B4i9EMQ4BQE
> 
> Thanks,
> 
>   Emilio
> 

Re: [Qemu-devel] [PATCH 2/5] cpus: use atomic_read to read seqlock-protected variables

[Emilio G. Cota]

On Mon, Oct 10, 2016 at 15:59:04 +0200, Paolo Bonzini wrote:
> There is a data race if the variable is written concurrently to the
> read.  In C11 this has undefined behavior.  Use atomic_read.  The
> write side does not need atomic_set, because it is protected by a
> mutex.

Is tsan happy with the way seqlocks are written right now?

According to this message I just found by Dmitry Vyukov, tsan
shouldn't be. Note however that the message is from April'15,
so it might be outdated:

Dmitry Vyukov wrote:
> 1. Tsan is bad at handling stand-alone memory barriers. 

> And here is a way to express seqlock that is both correct, is 
> understood by tsan and is no overhead on x86: 
> 
> // writer 
> atomic_store(, seq+1, memory_order_relaxed); 
> atomic_store([0], ..., memory_order_release); 
> ... 
> atomic_store([N], ..., memory_order_release); 
> atomic_store(, seq+1, memory_order_release); 
> 
> // reader 
> atomic_load(, memory_order_acquire); 
> d0 = atomic_load([0], memory_order_acquire); 
> ... 
> dN = atomic_load([N], memory_order_acquire); 
> atomic_load(, memory_order_relaxed); 

Source: https://groups.google.com/forum/#!topic/thread-sanitizer/B4i9EMQ4BQE

Thanks,

Emilio

Re: [Qemu-devel] [PATCH 2/5] cpus: use atomic_read to read seqlock-protected variables

[Alex Bennée]

Paolo Bonzini  writes:

> There is a data race if the variable is written concurrently to the
> read.  In C11 this has undefined behavior.  Use atomic_read.  The
> write side does not need atomic_set, because it is protected by a
> mutex.
>
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Alex Bennée 

> ---
>  cpus.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index b2fbe33..3fc2f6e 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -170,7 +170,8 @@ int64_t cpu_get_icount_raw(void)
>  static int64_t cpu_get_icount_locked(void)
>  {
>  int64_t icount = cpu_get_icount_raw();
> -return timers_state.qemu_icount_bias + cpu_icount_to_ns(icount);
> +int64_t ns = cpu_icount_to_ns(icount);
> +return atomic_read(_state.qemu_icount_bias) + ns;
>  }
>
>  int64_t cpu_get_icount(void)
> @@ -206,7 +207,7 @@ int64_t cpu_get_ticks(void)
>  }
>
>  ticks = timers_state.cpu_ticks_offset;
> -if (timers_state.cpu_ticks_enabled) {
> +if (atomic_read(_state.cpu_ticks_enabled)) {
>  ticks += cpu_get_host_ticks();
>  }
>
> @@ -225,8 +226,8 @@ static int64_t cpu_get_clock_locked(void)
>  {
>  int64_t time;
>
> -time = timers_state.cpu_clock_offset;
> -if (timers_state.cpu_ticks_enabled) {
> +time = atomic_read(_state.cpu_clock_offset);
> +if (atomic_read(_state.cpu_ticks_enabled)) {
>  time += get_clock();
>  }


--
Alex Bennée

[Qemu-devel] [PATCH 2/5] cpus: use atomic_read to read seqlock-protected variables

[Paolo Bonzini]

There is a data race if the variable is written concurrently to the
read.  In C11 this has undefined behavior.  Use atomic_read.  The
write side does not need atomic_set, because it is protected by a
mutex.

Signed-off-by: Paolo Bonzini 
---
 cpus.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index b2fbe33..3fc2f6e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -170,7 +170,8 @@ int64_t cpu_get_icount_raw(void)
 static int64_t cpu_get_icount_locked(void)
 {
 int64_t icount = cpu_get_icount_raw();
-return timers_state.qemu_icount_bias + cpu_icount_to_ns(icount);
+int64_t ns = cpu_icount_to_ns(icount);
+return atomic_read(_state.qemu_icount_bias) + ns;
 }
 
 int64_t cpu_get_icount(void)
@@ -206,7 +207,7 @@ int64_t cpu_get_ticks(void)
 }
 
 ticks = timers_state.cpu_ticks_offset;
-if (timers_state.cpu_ticks_enabled) {
+if (atomic_read(_state.cpu_ticks_enabled)) {
 ticks += cpu_get_host_ticks();
 }
 
@@ -225,8 +226,8 @@ static int64_t cpu_get_clock_locked(void)
 {
 int64_t time;
 
-time = timers_state.cpu_clock_offset;
-if (timers_state.cpu_ticks_enabled) {
+time = atomic_read(_state.cpu_clock_offset);
+if (atomic_read(_state.cpu_ticks_enabled)) {
 time += get_clock();
 }
 
-- 
2.7.4