Paolo Bonzini <pbonz...@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