[Qemu-devel] [PATCH 1/3] atomics: Test __STDC_VERSION__ for C11 compat

[Qemu-devel] [PATCH 1/3] atomics: Test __STDC_VERSION__ for C11 compat

[Pranith Kumar]

This patch tries to do the Right Thing™ to test for C11 features,
which is to test __STDC_VERSION__.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/qemu/atomic.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 43b0645..d313e6b 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -60,7 +60,7 @@
         (unsigned short)1,                                                         \
       (expr)+0))))))
 
-#ifdef __ATOMIC_RELAXED
+#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L)
 /* For C11 atomic ops */
 
 /* Manual memory barriers
@@ -210,7 +210,7 @@
 #define atomic_and(ptr, n) ((void) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST))
 #define atomic_or(ptr, n)  ((void) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST))
 
-#else /* __ATOMIC_RELAXED */
+#else /* __STDC_VERSION__ */
 
 /*
  * We use GCC builtin if it's available, as that can use mfence on
@@ -405,5 +405,5 @@
 #define atomic_and(ptr, n)     ((void) __sync_fetch_and_and(ptr, n))
 #define atomic_or(ptr, n)      ((void) __sync_fetch_and_or(ptr, n))
 
-#endif /* __ATOMIC_RELAXED */
+#endif /* __STDC_VERSION__ */
 #endif /* QEMU_ATOMIC_H */
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] atomics: Use __atomic_*_n() variant primitives

[Pranith Kumar]

Use the __atomic_*_n() primitives which take the value as argument. It
is not necessary to store the value locally before calling the
primitive, hence saving us a stack store and load.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/qemu/atomic.h | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index d313e6b..fc2309f 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -103,8 +103,7 @@
 
 #define atomic_set(ptr, i)  do {                      \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
-    typeof(*ptr) _val = (i);                          \
-    __atomic_store(ptr, &_val, __ATOMIC_RELAXED);     \
+    __atomic_store_n(ptr, i, __ATOMIC_RELAXED);       \
 } while(0)
 
 /* See above: most compilers currently treat consume and acquire the
@@ -129,8 +128,7 @@
 
 #define atomic_rcu_set(ptr, i) do {                   \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
-    typeof(*ptr) _val = (i);                          \
-    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);     \
+    __atomic_store_n(ptr, i, __ATOMIC_RELEASE);       \
 } while(0)
 
 /* atomic_mb_read/set semantics map Java volatile variables. They are
@@ -153,9 +151,8 @@
 
 #define atomic_mb_set(ptr, i)  do {                     \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
-    typeof(*ptr) _val = (i);                            \
     smp_wmb();                                          \
-    __atomic_store(ptr, &_val, __ATOMIC_RELAXED);       \
+    __atomic_store_n(ptr, i, __ATOMIC_RELAXED);         \
     smp_mb();                                           \
 } while(0)
 #else
@@ -169,8 +166,7 @@
 
 #define atomic_mb_set(ptr, i)  do {                     \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
-    typeof(*ptr) _val = (i);                            \
-    __atomic_store(ptr, &_val, __ATOMIC_SEQ_CST);       \
+    __atomic_store_n(ptr, i, __ATOMIC_SEQ_CST);         \
 } while(0)
 #endif
 
@@ -179,9 +175,7 @@
 
 #define atomic_xchg(ptr, i)    ({                           \
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));       \
-    typeof_strip_qual(*ptr) _new = (i), _old;               \
-    __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
-    _old;                                                   \
+    __atomic_exchange_n(ptr, i, __ATOMIC_SEQ_CST);          \
 })
 
 /* Returns the eventual value, failed or not */
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/3] atomics: Use __atomic_*_n() variant primitives

[Paolo Bonzini]

On 24/08/2016 22:44, Pranith Kumar wrote:
> Use the __atomic_*_n() primitives which take the value as argument. It
> is not necessary to store the value locally before calling the
> primitive, hence saving us a stack store and load.

If you do this, you might as well do it for __atomic_load and
__atomic_compare_exchange.  However, you'd still need typeof_strip_qual
for __atomic_compare_exchange's "_old" local variable.

The question is, does this actually cause a change in generated code?  I
would be very surprised if it did, but then it's perfectly possible to
have a bug in GCC.

Paolo

Re: [Qemu-devel] [PATCH 2/3] atomics: Use __atomic_*_n() variant primitives

[Pranith Kumar]

Paolo Bonzini writes:

> On 24/08/2016 22:44, Pranith Kumar wrote:
>> Use the __atomic_*_n() primitives which take the value as argument. It
>> is not necessary to store the value locally before calling the
>> primitive, hence saving us a stack store and load.
>
> If you do this, you might as well do it for __atomic_load and
> __atomic_compare_exchange.  However, you'd still need typeof_strip_qual
> for __atomic_compare_exchange's "_old" local variable.

OK, I will update the patch doing the same for load and compare exchange.

>
> The question is, does this actually cause a change in generated code?  I
> would be very surprised if it did, but then it's perfectly possible to
> have a bug in GCC.

It looks like it does. I tested atomic_load() with previous and the
atomic_load_n() version and it generates the following code:

current:
  mov    0x200c4a(%rip),%eax        # 601030 <value>
  mov    %eax,-0x4(%rsp)
  mov    -0x4(%rsp),%eax

atomic_load_n():
  mov    0x200c4a(%rip),%eax        # 601030 <value>

So looks like it's worth it.

Thanks,
-- 
Pranith

[Qemu-devel] [PATCH 3/3] atomics: Remove redundant barrier()'s

[Pranith Kumar]

Remove the redundant barrier() after the fence as agreed in previous
discussion here:
https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg00489.html

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/qemu/atomic.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index fc2309f..7e557d5 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -72,16 +72,16 @@
  * Add one here, and similarly in smp_rmb() and smp_read_barrier_depends().
  */
 
-#define smp_mb()    ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); barrier(); })
-#define smp_wmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); barrier(); })
-#define smp_rmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); barrier(); })
+#define smp_mb()    ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
+#define smp_wmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); })
+#define smp_rmb()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); })
 
 /* Most compilers currently treat consume and acquire the same, but really
  * no processors except Alpha need a barrier here.  Leave it in if
  * using Thread Sanitizer to avoid warnings, otherwise optimize it away.
  */
 #if defined(__SANITIZE_THREAD__)
-#define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); barrier(); })
+#define smp_read_barrier_depends() ({ barrier(); __atomic_thread_fence(__ATOMIC_CONSUME); })
 #elsif defined(__alpha__)
 #define smp_read_barrier_depends()   asm volatile("mb":::"memory")
 #else
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/3] atomics: Test __STDC_VERSION__ for C11 compat

[Paolo Bonzini]

On 24/08/2016 22:44, Pranith Kumar wrote:
> This patch tries to do the Right Thing™ to test for C11 features,
> which is to test __STDC_VERSION__.

Compilers can provide atomics even for older standards, for example:

$ echo '__STDC_VERSION__' | gcc -E -x c - -std=gnu99 | tail -1
199901L
$ echo '__ATOMIC_RELAXED' | gcc -E -x c - -std=gnu99 | tail -1
0

Which is why the patch is not using __STDC_VERSION__.

Unfortunately, the C standard does not define a feature macro for
atomics; it provides __STDC_NO_ATOMICS__ but it is defined backwards.

Thanks,

Paolo

> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  include/qemu/atomic.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index 43b0645..d313e6b 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -60,7 +60,7 @@
>          (unsigned short)1,                                                         \
>        (expr)+0))))))
>  
> -#ifdef __ATOMIC_RELAXED
> +#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L)
>  /* For C11 atomic ops */
>  
>  /* Manual memory barriers
> @@ -210,7 +210,7 @@
>  #define atomic_and(ptr, n) ((void) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST))
>  #define atomic_or(ptr, n)  ((void) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST))
>  
> -#else /* __ATOMIC_RELAXED */
> +#else /* __STDC_VERSION__ */
>  
>  /*
>   * We use GCC builtin if it's available, as that can use mfence on
> @@ -405,5 +405,5 @@
>  #define atomic_and(ptr, n)     ((void) __sync_fetch_and_and(ptr, n))
>  #define atomic_or(ptr, n)      ((void) __sync_fetch_and_or(ptr, n))
>  
> -#endif /* __ATOMIC_RELAXED */
> +#endif /* __STDC_VERSION__ */
>  #endif /* QEMU_ATOMIC_H */
>