Re: [Qemu-devel] [PATCH 1/2] atomics: do not use __atomic primitives for RCU atomics

[Alex Bennée]

Emilio G. Cota <address@hidden> writes:

> Commit a0aa44b4 ("include/qemu/atomic.h: default to __atomic functions")
> set all atomics to default (on recent GCC versions) to __atomic primitives.
>
> In the process, the atomic_rcu_read/set were converted to implement
> consume/release semantics, respectively. This is inefficient; for
> correctness and maximum performance we only need an smp_barrier_depends
> for reads, and an smp_wmb for writes. Fix it by using the original
> definition of these two primitives for all compilers.
>
> Note that in case these semantics were necessary to avoid false
> positives under Thread Sanitizer, we could have them defined as such
> under #ifdef __SANITIZE_THREAD__. I tried running QEMU under tsan
> to explore this, but unfortunately I couldn't get it to work (tsan dies
> with an "unexpected memory mapping"). I suspect though that the
> atomic_read/set embedded in atomic_rcu_read/set is enough for tsan,
> though.

For tsan runs you need to re-build with:

  ./configure --cc=gcc --extra-cflags="-pie -fPIE -fsanitize=thread" 
--with-coroutine=gthread

Specifically the coroutine ucontext messing really confuses TSAN.

>
> Signed-off-by: Emilio G. Cota <address@hidden>
> ---
>  include/qemu/atomic.h | 99 
> ++++++++++++++++++++++-----------------------------
>  1 file changed, 43 insertions(+), 56 deletions(-)
>
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index 5bc4d6c..4d62425 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -56,22 +56,6 @@
>      __atomic_store(ptr, &_val, __ATOMIC_RELAXED);     \
>  } while(0)
>
> -/* Atomic RCU operations imply weak memory barriers */
> -
> -#define atomic_rcu_read(ptr)                          \
> -    ({                                                \
> -    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
> -    typeof(*ptr) _val;                                \
> -    __atomic_load(ptr, &_val, __ATOMIC_CONSUME);      \
> -    _val;                                             \
> -    })
> -
> -#define atomic_rcu_set(ptr, i) do {                   \
> -    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
> -    typeof(*ptr) _val = (i);                          \
> -    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);     \
> -} while(0)
> -
>  /* atomic_mb_read/set semantics map Java volatile variables. They are
>   * less expensive on some platforms (notably POWER & ARMv7) than fully
>   * sequentially consistent operations.
> @@ -242,46 +226,6 @@
>  #define atomic_read(ptr)       (*(__typeof__(*ptr) volatile*) (ptr))
>  #define atomic_set(ptr, i)     ((*(__typeof__(*ptr) volatile*) (ptr)) = (i))
>
> -/**
> - * atomic_rcu_read - reads a RCU-protected pointer to a local variable
> - * into a RCU read-side critical section. The pointer can later be safely
> - * dereferenced within the critical section.
> - *
> - * This ensures that the pointer copy is invariant thorough the whole 
> critical
> - * section.
> - *
> - * Inserts memory barriers on architectures that require them (currently only
> - * Alpha) and documents which pointers are protected by RCU.
> - *
> - * atomic_rcu_read also includes a compiler barrier to ensure that
> - * value-speculative optimizations (e.g. VSS: Value Speculation
> - * Scheduling) does not perform the data read before the pointer read
> - * by speculating the value of the pointer.
> - *
> - * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg().
> - */
> -#define atomic_rcu_read(ptr)    ({                \
> -    typeof(*ptr) _val = atomic_read(ptr);         \
> -    smp_read_barrier_depends();                   \
> -    _val;                                         \
> -})
> -
> -/**
> - * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure
> - * meant to be read by RCU read-side critical sections.
> - *
> - * Documents which pointers will be dereferenced by RCU read-side critical
> - * sections and adds the required memory barriers on architectures requiring
> - * them. It also makes sure the compiler does not reorder code initializing 
> the
> - * data structure before its publication.
> - *
> - * Should match atomic_rcu_read().
> - */
> -#define atomic_rcu_set(ptr, i)  do {              \
> -    smp_wmb();                                    \
> -    atomic_set(ptr, i);                           \
> -} while (0)
> -
>  /* These have the same semantics as Java volatile variables.
>   * See http://gee.cs.oswego.edu/dl/jmm/cookbook.html:
>   * "1. Issue a StoreStore barrier (wmb) before each volatile store."
> @@ -345,4 +289,47 @@
>  #define atomic_or(ptr, n)      ((void) __sync_fetch_and_or(ptr, n))
>
>  #endif /* __ATOMIC_RELAXED */
> +
> +/**
> + * atomic_rcu_read - reads a RCU-protected pointer to a local variable
> + * into a RCU read-side critical section. The pointer can later be safely
> + * dereferenced within the critical section.
> + *
> + * This ensures that the pointer copy is invariant thorough the whole 
> critical
> + * section.
> + *
> + * Inserts memory barriers on architectures that require them (currently only
> + * Alpha) and documents which pointers are protected by RCU.
> + *
> + * atomic_rcu_read also includes a compiler barrier to ensure that
> + * value-speculative optimizations (e.g. VSS: Value Speculation
> + * Scheduling) does not perform the data read before the pointer read
> + * by speculating the value of the pointer.
> + *
> + * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg().
> + */
> +#define atomic_rcu_read(ptr)    ({                    \
> +    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
> +    typeof(*ptr) _val = atomic_read(ptr);             \
> +    smp_read_barrier_depends();                       \
> +    _val;                                             \
> +})
> +
> +/**
> + * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure
> + * meant to be read by RCU read-side critical sections.
> + *
> + * Documents which pointers will be dereferenced by RCU read-side critical
> + * sections and adds the required memory barriers on architectures requiring
> + * them. It also makes sure the compiler does not reorder code initializing 
> the
> + * data structure before its publication.
> + *
> + * Should match atomic_rcu_read().
> + */
> +#define atomic_rcu_set(ptr, i)  do {                  \
> +    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
> +    smp_wmb();                                        \
> +    atomic_set(ptr, i);                               \
> +} while (0)
> +
>  #endif /* __QEMU_ATOMIC_H */


--
Alex Bennée