Re: [Qemu-devel] [PATCH 2/5] cpus: use atomic_read to read seqlock-protected variables
> 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, seq+1, memory_order_relaxed); > > atomic_store(&data[0], ..., memory_order_release); > > ... > > atomic_store(&data[N], ..., memory_order_release); > > atomic_store(&seq, seq+1, memory_order_release); > > > > // reader > > atomic_load(&seq, memory_order_acquire); > > d0 = atomic_load(&data[0], memory_order_acquire); > > ... > > dN = atomic_load(&data[N], memory_order_acquire); > > atomic_load(&seq, 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
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, seq+1, memory_order_relaxed); > atomic_store(&data[0], ..., memory_order_release); > ... > atomic_store(&data[N], ..., memory_order_release); > atomic_store(&seq, seq+1, memory_order_release); > > // reader > atomic_load(&seq, memory_order_acquire); > d0 = atomic_load(&data[0], memory_order_acquire); > ... > dN = atomic_load(&data[N], memory_order_acquire); > atomic_load(&seq, 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
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(&timers_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(&timers_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(&timers_state.cpu_clock_offset); > +if (atomic_read(&timers_state.cpu_ticks_enabled)) { > time += get_clock(); > } -- Alex Bennée