| Message ID | 1460730231-1184-7-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Richard Henderson <rth@twiddle.net> writes: > On 04/15/2016 07:23 AM, Alex Bennée wrote: >> +#define atomic_bool_cmpxchg(ptr, old, new) \ >> + ({ \ >> + typeof(*ptr) _old = (old), _new = (new); \ >> + bool r; \ >> + r = __atomic_compare_exchange(ptr, &_old, &_new, false, \ >> + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \ >> + r; \ >> + }) > > How are you thinking this will be used? If a loop like > > do { > old = atomic_read (ptr); > new = f(old); > } while (!atomic_bool_cmpxchg(ptr, old, new)); > > then it's usually helpful to use a weak compare_exchange (s/false/true/ above). > This will produce one loop for ll/sc architectures instead of two. I used it to make Fred's STREX code a little neater: if (len == 1 << size) { oldval = (uint8_t)env->exclusive_val; result = atomic_bool_cmpxchg(p, oldval, (uint8_t)newval); } address_space_unmap(cs->as, p, len, true, result ? len : 0); Instead of: if (len == 1 << size) { oldval = (uint8_t)env->exclusive_val; result = (atomic_cmpxchg(p, oldval, (uint8_t)newval) == oldval); } address_space_unmap(cs->as, p, len, true, result ? len : 0); But now I'm wondering if there is a race in there. I'll have to look closer. > > > r~ -- Alex Bennée
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 15/04/16 17:23, Alex Bennée wrote: >> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h >> index 5dba7db..94e7110 100644 >> --- a/include/qemu/atomic.h >> +++ b/include/qemu/atomic.h >> @@ -123,6 +123,16 @@ >> _old; \ >> }) >> >> +#define atomic_bool_cmpxchg(ptr, old, new) \ >> + ({ \ >> + typeof(*ptr) _old = (old), _new = (new); \ >> + bool r; \ >> + r = __atomic_compare_exchange(ptr, &_old, &_new, false, \ >> + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \ >> + r; \ >> + }) >> + >> + > > Could be more simple: > > #define atomic_bool_cmpxchg(ptr, old, new) \ > ({ \ > typeof(*ptr) _old = (old), _new = (new); \ > __atomic_compare_exchange(ptr, &_old, &_new, false, \ > __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \ > }) OK that makes sense. I'll have to ask my toolchain colleague what the rules are for results from {} blocks. -- Alex Bennée
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index 5dba7db..94e7110 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -123,6 +123,16 @@ _old; \ }) +#define atomic_bool_cmpxchg(ptr, old, new) \ + ({ \ + typeof(*ptr) _old = (old), _new = (new); \ + bool r; \ + r = __atomic_compare_exchange(ptr, &_old, &_new, false, \ + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); \ + r; \ + }) + + /* Provide shorter names for GCC atomic builtins, return old value */ #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) @@ -327,6 +337,7 @@ #define atomic_fetch_and __sync_fetch_and_and #define atomic_fetch_or __sync_fetch_and_or #define atomic_cmpxchg __sync_val_compare_and_swap +#define atomic_bool_cmpxchg __sync_bool_compare_and_swap #define atomic_dec_fetch(ptr) __sync_sub_and_fetch(ptr, 1)
This allows for slightly neater code when checking for success of a cmpxchg operation. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/qemu/atomic.h | 11 +++++++++++ 1 file changed, 11 insertions(+) -- 2.7.4