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

["Alex Bennée" <alex.bennee@linaro.org>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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).

The original comment mentioned both POWER and ARM so I wondering if we
should also special case for the ARMv7?

>
>> +/* Returns the eventual value, failed or not */

Yeah this comment in bogus.

>> +#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?

My mistake, I was having a worry that we weren't following the old
semantics. In fact having read even more closely I understand that _old is
updated by the __atomic function if the update fails. In fact _old is a
poor name because its _expected at the start and _current in the case it
fails. In fact:

    This compares the contents of *ptr with the contents of *expected. If
    equal, the operation is a read-modify-write operation that writes
    desired into *ptr. If they are not equal, the operation is a read and
    the current contents of *ptr are written into *expected. <snip>...

    If desired is written into *ptr then true is returned and memory is
    affected according to the memory order specified by success_memorder.
    There are no restrictions on what memory order can be used here.

I was wondering if this was subtly different from the old
__sync_val_compare_and_swap:

    The “val” version returns the contents of *ptr before the operation.

I think we are OK because if cmpxchg succeeds _old was by definition
what was already there but it is confusing and leads to funny code like
this:

    if (atomic_cmpxchg(&data[i].n, 0, 3) == 0) {
        data[i].ret = -ECANCELED;
        ...

and

    if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
       ...

Which might be easier to read if atomic_cmpxchg used the bool
semantics, i.e. return true for a successful cmpxchg.

The old code even has a atomic_bool_cmpxchg which no one seems to use. I
wonder if the correct solution is to convert atomic_cmpxchg calls to use
atomic_cmpxchg_bool calls and remove atomic_cmpxchg from atomic.h?

What do you think?

>
> Paolo


--
Alex Bennée