[Qemu-devel] [PATCH 1/2] atomics: do not use __atomic primitives for RCU atomics

[Emilio G. Cota]

Commit a0aa44b4 ("include/qemu/atomic.h: default to __atomic functions")
set all atomics to default (on recent GCC versions) to __atomic primitives.

In the process, the atomic_rcu_read/set were converted to implement
consume/release semantics, respectively. This is inefficient; for
correctness and maximum performance we only need an smp_barrier_depends
for reads, and an smp_wmb for writes. Fix it by using the original
definition of these two primitives for all compilers.

Note that in case these semantics were necessary to avoid false
positives under Thread Sanitizer, we could have them defined as such
under #ifdef __SANITIZE_THREAD__. I tried running QEMU under tsan
to explore this, but unfortunately I couldn't get it to work (tsan dies
with an "unexpected memory mapping"). I suspect though that the
atomic_read/set embedded in atomic_rcu_read/set is enough for tsan,
though.

Signed-off-by: Emilio G. Cota <address@hidden>
---
 include/qemu/atomic.h | 99 ++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 56 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 5bc4d6c..4d62425 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -56,22 +56,6 @@
     __atomic_store(ptr, &_val, __ATOMIC_RELAXED);     \
 } while(0)
 
-/* Atomic RCU operations imply weak memory barriers */
-
-#define atomic_rcu_read(ptr)                          \
-    ({                                                \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
-    typeof(*ptr) _val;                                \
-    __atomic_load(ptr, &_val, __ATOMIC_CONSUME);      \
-    _val;                                             \
-    })
-
-#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);     \
-} while(0)
-
 /* atomic_mb_read/set semantics map Java volatile variables. They are
  * less expensive on some platforms (notably POWER & ARMv7) than fully
  * sequentially consistent operations.
@@ -242,46 +226,6 @@
 #define atomic_read(ptr)       (*(__typeof__(*ptr) volatile*) (ptr))
 #define atomic_set(ptr, i)     ((*(__typeof__(*ptr) volatile*) (ptr)) = (i))
 
-/**
- * atomic_rcu_read - reads a RCU-protected pointer to a local variable
- * into a RCU read-side critical section. The pointer can later be safely
- * dereferenced within the critical section.
- *
- * This ensures that the pointer copy is invariant thorough the whole critical
- * section.
- *
- * Inserts memory barriers on architectures that require them (currently only
- * Alpha) and documents which pointers are protected by RCU.
- *
- * atomic_rcu_read also includes a compiler barrier to ensure that
- * value-speculative optimizations (e.g. VSS: Value Speculation
- * Scheduling) does not perform the data read before the pointer read
- * by speculating the value of the pointer.
- *
- * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg().
- */
-#define atomic_rcu_read(ptr)    ({                \
-    typeof(*ptr) _val = atomic_read(ptr);         \
-    smp_read_barrier_depends();                   \
-    _val;                                         \
-})
-
-/**
- * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure
- * meant to be read by RCU read-side critical sections.
- *
- * Documents which pointers will be dereferenced by RCU read-side critical
- * sections and adds the required memory barriers on architectures requiring
- * them. It also makes sure the compiler does not reorder code initializing the
- * data structure before its publication.
- *
- * Should match atomic_rcu_read().
- */
-#define atomic_rcu_set(ptr, i)  do {              \
-    smp_wmb();                                    \
-    atomic_set(ptr, i);                           \
-} while (0)
-
 /* These have the same semantics as Java volatile variables.
  * See http://gee.cs.oswego.edu/dl/jmm/cookbook.html:
  * "1. Issue a StoreStore barrier (wmb) before each volatile store."
@@ -345,4 +289,47 @@
 #define atomic_or(ptr, n)      ((void) __sync_fetch_and_or(ptr, n))
 
 #endif /* __ATOMIC_RELAXED */
+
+/**
+ * atomic_rcu_read - reads a RCU-protected pointer to a local variable
+ * into a RCU read-side critical section. The pointer can later be safely
+ * dereferenced within the critical section.
+ *
+ * This ensures that the pointer copy is invariant thorough the whole critical
+ * section.
+ *
+ * Inserts memory barriers on architectures that require them (currently only
+ * Alpha) and documents which pointers are protected by RCU.
+ *
+ * atomic_rcu_read also includes a compiler barrier to ensure that
+ * value-speculative optimizations (e.g. VSS: Value Speculation
+ * Scheduling) does not perform the data read before the pointer read
+ * by speculating the value of the pointer.
+ *
+ * Should match atomic_rcu_set(), atomic_xchg(), atomic_cmpxchg().
+ */
+#define atomic_rcu_read(ptr)    ({                    \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
+    typeof(*ptr) _val = atomic_read(ptr);             \
+    smp_read_barrier_depends();                       \
+    _val;                                             \
+})
+
+/**
+ * atomic_rcu_set - assigns (publicizes) a pointer to a new data structure
+ * meant to be read by RCU read-side critical sections.
+ *
+ * Documents which pointers will be dereferenced by RCU read-side critical
+ * sections and adds the required memory barriers on architectures requiring
+ * them. It also makes sure the compiler does not reorder code initializing the
+ * data structure before its publication.
+ *
+ * Should match atomic_rcu_read().
+ */
+#define atomic_rcu_set(ptr, i)  do {                  \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
+    smp_wmb();                                        \
+    atomic_set(ptr, i);                               \
+} while (0)
+
 #endif /* __QEMU_ATOMIC_H */
-- 
2.5.0