| Message ID | 1460730231-1184-7-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. r~
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
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); \ }) Kind regards, Sergey
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
On 06/03/2016 01:12 PM, 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; \ >>> + }) >>> + >>> + >> >> 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. It's a gcc extension, and the rule is that the value of the overall ({}) is the value of the last statement in the block. No need for a temporary variable 'r' if you can just use __atomic_compare_exchange() as the last statement.
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(+)