Re: [Qemu-devel] [PATCH v2 1/2] add a header file for atomic operations

[Richard Henderson <rth@twiddle.net>]

On 06/16/2013 04:21 AM, Liu Ping Fan wrote:
> +#if QEMU_GNUC_PREREQ(4, 8)
> +#ifndef __ATOMIC_RELAXED
> +#define __ATOMIC_RELAXED 0
> +#endif

Why all the ifdefs?  If __atomic support is present, then __ATOMIC defines will
exist.

> +/* Compiler barrier */
> +#define barrier()   ({ asm volatile("" ::: "memory"); (void)0; })
> +
> +#if !QEMU_GNUC_PREREQ(4, 8)

With C++11 atomic support we should define barrier as

  __atomic_signal_fence(__ATOMIC_ACQ_REL)

>  #if defined(__powerpc64__)
> -#define smp_rmb()   asm volatile("lwsync" ::: "memory")
> +#define smp_rmb()   ({ asm volatile("lwsync" ::: "memory"); (void)0; })
>  #else
> -#define smp_rmb()   asm volatile("sync" ::: "memory")
> +#define smp_rmb()   ({ asm volatile("sync" ::: "memory"); (void)0; })
>  #endif

Err... lwsync corresponds to ACQ_REL consistency, while sync corresponds to
SEQ_CST consistency.  The newly added document says you want SEQ_CST while the
old code implies ACQ_REL.

Which is true?

> +#ifndef smp_mb
> +#define smp_mb()    __sync_synchronize()
> +#endif

Use __atomic_thread_fence here for completeness?

> +#ifndef atomic_read
> +#define atomic_read(ptr)       (*(__typeof__(*ptr) *volatile) (ptr))
>  #endif
>  
> +#ifndef atomic_set
> +#define atomic_set(ptr, i)     ((*(__typeof__(*ptr) *volatile) (ptr)) = (i))
> +#endif

Use

__atomic_load(..., __ATOMIC_RELAXED)
__atomic_store(..., __ATOMIC_RELAXED)

?

> + * And for the few ia64 lovers that exist, an atomic_mb_read is a ld.acq,
> + * while an atomic_mb_set is a st.rel followed by a memory barrier.
...
> + */
> +#ifndef atomic_mb_read
> +#if QEMU_GNUC_PREREQ(4, 8)
> +#define atomic_mb_read(ptr)       ({             \
> +    typeof(*ptr) _val;                           \
> +    __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST); \
> +    _val;                                        \
> +})
> +#else
> +#define atomic_mb_read(ptr)    ({           \
> +    typeof(*ptr) _val = atomic_read(ptr);   \
> +    smp_rmb();                              \
> +    _val;                                   \

This latter definition is ACQUIRE not SEQ_CST (except for ia64).  Without
load_acquire, one needs barriers before and after the atomic_read in order to
implement SEQ_CST.

So again I have to ask, what semantics are you actually looking for here?

> +#define atomic_mb_set(ptr, i)  do {         \
> +    smp_wmb();                              \
> +    atomic_set(ptr, i);                     \
> +    smp_mb();                               \
> +} while (0)

Really only a smp_wmb?  What about outstanding loads?


r~