[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 3/5] include/qemu/atomic.h: default to __atom
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v1 3/5] include/qemu/atomic.h: default to __atomic functions |
Date: |
Fri, 29 Jan 2016 16:06:04 +0000 |
User-agent: |
mu4e 0.9.17; emacs 25.0.50.8 |
Paolo Bonzini <address@hidden> 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
- [Qemu-devel] [PATCH v1 2/5] configure: ensure ldflags propagated to config_host, (continued)