Patchwork atomic: using memory_order_relaxed for refcnt inc/dec ops

login
register
mail settings
Submitter pingfan liu
Date July 12, 2013, 6:32 a.m.
Message ID <1373610771-11819-1-git-send-email-pingfank@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/258693/
State New
Headers show

Comments

pingfan liu - July 12, 2013, 6:32 a.m.
Refcnt's atomic inc/dec ops are frequent and its idiom need no seq_cst
order. So to get better performance, it worth to adopt _relaxed
other than _seq_cst memory model on them.

We resort to gcc builtins. If gcc supports C11 memory model, __atomic_*
buitlins is used, otherwise __sync_* builtins.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/atomic.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Richard Henderson - July 12, 2013, 4:24 p.m.
On 07/11/2013 11:32 PM, Liu Ping Fan wrote:
> Refcnt's atomic inc/dec ops are frequent and its idiom need no seq_cst
> order. So to get better performance, it worth to adopt _relaxed
> other than _seq_cst memory model on them.

You'd need to update the documentation then.  As it stands, what you've written
looks like a bug.

> +#ifndef _GLIBCXX_ATOMIC_BUILTINS

This will never be defined.  It's private to the libstdc++ implementation.  See
how we've defined things using __atomic elsewhere in the file, looking at one
of the __ATOMIC defines.

And in either case, it's better form to use positive tests than negative ones.
I.e. #ifdef rather than #ifndef

>  #define atomic_fetch_inc(ptr)  __sync_fetch_and_add(ptr, 1)
>  #define atomic_fetch_dec(ptr)  __sync_fetch_and_add(ptr, -1)

I'd prefer atomic_fetch_inc_relaxed, as that's more self-documenting.

But I'll re-iterate the necessity of documentation in this area.


r~
pingfan liu - July 14, 2013, 2:18 a.m.
On Sat, Jul 13, 2013 at 12:24 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 07/11/2013 11:32 PM, Liu Ping Fan wrote:
>> Refcnt's atomic inc/dec ops are frequent and its idiom need no seq_cst
>> order. So to get better performance, it worth to adopt _relaxed
>> other than _seq_cst memory model on them.
>
> You'd need to update the documentation then.  As it stands, what you've written
> looks like a bug.
>
Ok, will update atomic.txt

>> +#ifndef _GLIBCXX_ATOMIC_BUILTINS
>
> This will never be defined.  It's private to the libstdc++ implementation.  See
> how we've defined things using __atomic elsewhere in the file, looking at one
> of the __ATOMIC defines.
>
Oh, I misunderstood the description of _GLIBCXX_ATOMIC_BUILTINS.  Got
it, thanks.

> And in either case, it's better form to use positive tests than negative ones.
> I.e. #ifdef rather than #ifndef
>
Will fix.
>>  #define atomic_fetch_inc(ptr)  __sync_fetch_and_add(ptr, 1)
>>  #define atomic_fetch_dec(ptr)  __sync_fetch_and_add(ptr, -1)
>
> I'd prefer atomic_fetch_inc_relaxed, as that's more self-documenting.
>
But if _relaxed not supported, it will fall back on _seq_cst, then
atomic_fetch_inc_relaxed will be wrong named?

Thanks and regards,
Pingfan

> But I'll re-iterate the necessity of documentation in this area.
>
>
> r~

Patch

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 0aa8913..1f474b7 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -183,8 +183,15 @@ 
 #endif
 
 /* Provide shorter names for GCC atomic builtins.  */
+#ifndef _GLIBCXX_ATOMIC_BUILTINS
+/* close to C11 memory_order_seq_cst */
 #define atomic_fetch_inc(ptr)  __sync_fetch_and_add(ptr, 1)
 #define atomic_fetch_dec(ptr)  __sync_fetch_and_add(ptr, -1)
+#else
+/* C11 memory_order_relaxed */
+#define atomic_fetch_inc(ptr)  __atomic_fetch_add(ptr, 1, __ATOMIC_RELAXED)
+#define atomic_fetch_dec(ptr)  __atomic_fetch_add(ptr, -1, __ATOMIC_RELAXED)
+#endif
 #define atomic_fetch_add       __sync_fetch_and_add
 #define atomic_fetch_sub       __sync_fetch_and_sub
 #define atomic_fetch_and       __sync_fetch_and_and
@@ -192,8 +199,15 @@ 
 #define atomic_cmpxchg         __sync_val_compare_and_swap
 
 /* And even shorter names that return void.  */
+#ifndef _GLIBCXX_ATOMIC_BUILTINS
+/* close to C11 memory_order_seq_cst */
 #define atomic_inc(ptr)        ((void) __sync_fetch_and_add(ptr, 1))
 #define atomic_dec(ptr)        ((void) __sync_fetch_and_add(ptr, -1))
+#else
+/* C11 memory_order_relaxed */
+#define atomic_inc(ptr)        ((void) __atomic_fetch_add(ptr, 1, __ATOMIC_RELAXED))
+#define atomic_dec(ptr)        ((void) __atomic_fetch_add(ptr, -1, __ATOMIC_RELAXED))
+#endif
 #define atomic_add(ptr, n)     ((void) __sync_fetch_and_add(ptr, n))
 #define atomic_sub(ptr, n)     ((void) __sync_fetch_and_sub(ptr, n))
 #define atomic_and(ptr, n)     ((void) __sync_fetch_and_and(ptr, n))