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~
