Re: [Qemu-devel] [PATCH v1 3/5] include/qemu/atomic.h: default to __atomic functions

[Paolo Bonzini]

On 28/01/2016 11:15, Alex Bennée wrote:
> +/* atomic_mb_read/set semantics map Java volatile variables. They are
> + * less expensive on some platforms (notably POWER & ARM) than fully
> + * sequentially consistent operations.
> + *
> + * As long as they are used as paired operations they are safe to
> + * use. See docs/atomic.txt for more discussion.
> + */
> +
> +#define atomic_mb_read(ptr)                             \
> +    ({                                                  \
> +    typeof(*ptr) _val;                                  \
> +     __atomic_load(ptr, &_val, __ATOMIC_RELAXED);       \
> +     smp_rmb();                                         \
> +    _val;                                               \
> +    })
> +
> +#define atomic_mb_set(ptr, i)  do {                     \
> +    typeof(*ptr) _val = (i);                            \
> +    smp_wmb();                                          \
> +    __atomic_store(ptr, &_val, __ATOMIC_RELAXED);       \
> +    smp_mb();                                           \
> +} while(0)

Great... I'll change this to

#if defined(_ARCH_PPC)
#define atomic_mb_read(ptr)                             \
    ({                                                  \
    typeof(*ptr) _val;                                  \
     __atomic_load(ptr, &_val, __ATOMIC_RELAXED);       \
     smp_rmb();                                         \
    _val;                                               \
    })

#define atomic_mb_set(ptr, i)  do {                     \
    typeof(*ptr) _val = (i);                            \
    smp_wmb();                                          \
    __atomic_store(ptr, &_val, __ATOMIC_RELAXED);       \
    smp_mb();                                           \
} while(0)
#else
#define atomic_mb_read(ptr)                       \
    ({                                            \
    typeof(*ptr) _val;                            \
     __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST); \
    _val;                                         \
    })

#define atomic_mb_set(ptr, i)  do {               \
    typeof(*ptr) _val = (i);                      \
    __atomic_store(ptr, &_val, __ATOMIC_SEQ_CST); \
} while(0)
#endif

since this benefits x86 (which can generate mov/xchg respectively) and
aarch64 (where atomic_mb_read/atomic_mb_set map directly to ldar/stlr).

> +/* Returns the eventual value, failed or not */
> +#define atomic_cmpxchg(ptr, old, new)                                   \
> +    ({                                                                  \
> +    typeof(*ptr) _old = (old), _new = (new);                            \
> +    __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
> +                              __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
> +    _old; /* can this race if cmpxchg not used elsewhere? */            \
> +    })

How so?

Paolo