atomics: eliminate mb_read/mb_set

qatomic_mb_read and qatomic_mb_set were the very first atomic primitives
introduced for QEMU; their semantics are unclear and they provide a false
sense of safety.

The last use of qatomic_mb_read() has been removed, so delete it.
qatomic_mb_set() instead can survive as an optimized
qatomic_set()+smp_mb(), similar to Linux's smp_store_mb(), but
rename it to qatomic_set_mb() to match the order of the two
operations.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 1cf4f1fa227e93d0ba47f10e3e91c0bf999a5ecd..42086525d7a66b24c3ee0f4ba7bbb926491dd978 100644 (file)
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -774,7 +774,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
      * Ensure zeroing happens before reading cpu->exit_request or
      * cpu->interrupt_request (see also smp_wmb in cpu_exit())
      */
-    qatomic_mb_set(&cpu_neg(cpu)->icount_decr.u16.high, 0);
+    qatomic_set_mb(&cpu_neg(cpu)->icount_decr.u16.high, 0);
 
     if (unlikely(qatomic_read(&cpu->interrupt_request))) {
         int interrupt_request;

diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
index 5d72c9b1bd3c25902f78ca22ea4e8a8be10ac2a8..b320ff003730f5da8ce24562ce712618a6b261fe 100644 (file)
--- a/accel/tcg/tcg-accel-ops-mttcg.c
+++ b/accel/tcg/tcg-accel-ops-mttcg.c
@@ -119,7 +119,7 @@ static void *mttcg_cpu_thread_fn(void *arg)
             }
         }
 
-        qatomic_mb_set(&cpu->exit_request, 0);
+        qatomic_set_mb(&cpu->exit_request, 0);
         qemu_wait_io_event(cpu);
     } while (!cpu->unplug || cpu_can_run(cpu));
 

diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index 70b9b89073c250ba356e5e7fe3a5330885cbe463..23e4d0f4527eb3fb29d149cbb6c1b5a401704e86 100644 (file)
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -244,7 +244,7 @@ static void *rr_cpu_thread_fn(void *arg)
 
         while (cpu && cpu_work_list_empty(cpu) && !cpu->exit_request) {
             /* Store rr_current_cpu before evaluating cpu_can_run().  */
-            qatomic_mb_set(&rr_current_cpu, cpu);
+            qatomic_set_mb(&rr_current_cpu, cpu);
 
             current_cpu = cpu;
 
@@ -287,7 +287,7 @@ static void *rr_cpu_thread_fn(void *arg)
         qatomic_set(&rr_current_cpu, NULL);
 
         if (cpu && cpu->exit_request) {
-            qatomic_mb_set(&cpu->exit_request, 0);
+            qatomic_set_mb(&cpu->exit_request, 0);
         }
 
         if (icount_enabled() && all_cpu_threads_idle()) {

diff --git a/docs/devel/atomics.rst b/docs/devel/atomics.rst
index 248076375bfe3cfff683e9d8a23720f03fa919be..ff9b5ee30c885c488a234b1b7ca4d1bf1b15c4d1 100644 (file)
--- a/docs/devel/atomics.rst
+++ b/docs/devel/atomics.rst
@@ -102,28 +102,10 @@ Similar operations return the new value of ``*ptr``::
     typeof(*ptr) qatomic_or_fetch(ptr, val)
     typeof(*ptr) qatomic_xor_fetch(ptr, val)
 
-``qemu/atomic.h`` also provides loads and stores that cannot be reordered
-with each other::
+``qemu/atomic.h`` also provides an optimized shortcut for
+``qatomic_set`` followed by ``smp_mb``::
 
-    typeof(*ptr) qatomic_mb_read(ptr)
-    void         qatomic_mb_set(ptr, val)
-
-However these do not provide sequential consistency and, in particular,
-they do not participate in the total ordering enforced by
-sequentially-consistent operations.  For this reason they are deprecated.
-They should instead be replaced with any of the following (ordered from
-easiest to hardest):
-
-- accesses inside a mutex or spinlock
-
-- lightweight synchronization primitives such as ``QemuEvent``
-
-- RCU operations (``qatomic_rcu_read``, ``qatomic_rcu_set``) when publishing
-  or accessing a new version of a data structure
-
-- other atomic accesses: ``qatomic_read`` and ``qatomic_load_acquire`` for
-  loads, ``qatomic_set`` and ``qatomic_store_release`` for stores, ``smp_mb``
-  to forbid reordering subsequent loads before a store.
+    void         qatomic_set_mb(ptr, val)
 
 
 Weak atomic access and manual memory barriers
@@ -523,8 +505,7 @@ and memory barriers, and the equivalents in QEMU:
       | ::                             |
       |                                |
       |   a = qatomic_read(&x);        |
-      |   qatomic_set(&x, a + 2);      |
-      |   smp_mb();                    |
+      |   qatomic_set_mb(&x, a + 2);   |
       |   b = qatomic_read(&y);        |
       +--------------------------------+
 

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index f85834ee8b20d80e9f049377d20f2e0acaf03fde..d95612f7a084ef11af68ddd1733285a2edba3e63 100644 (file)
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -259,24 +259,17 @@
 # define smp_mb__after_rmw() smp_mb()
 #endif
 
-/* qatomic_mb_read/set semantics map Java volatile variables. They are
- * less expensive on some platforms (notably POWER) than fully
- * sequentially consistent operations.
- *
- * As long as they are used as paired operations they are safe to
- * use. See docs/devel/atomics.rst for more discussion.
+/*
+ * On some architectures, qatomic_set_mb is more efficient than a store
+ * plus a fence.
  */
 
-#define qatomic_mb_read(ptr)                             \
-    qatomic_load_acquire(ptr)
-
 #if !defined(QEMU_SANITIZE_THREAD) && \
     (defined(__i386__) || defined(__x86_64__) || defined(__s390x__))
-/* This is more efficient than a store plus a fence.  */
-# define qatomic_mb_set(ptr, i) \
+# define qatomic_set_mb(ptr, i) \
     ({ (void)qatomic_xchg(ptr, i); smp_mb__after_rmw(); })
 #else
-# define qatomic_mb_set(ptr, i) \
+# define qatomic_set_mb(ptr, i) \
    ({ qatomic_store_release(ptr, i); smp_mb(); })
 #endif
 

diff --git a/monitor/qmp.c b/monitor/qmp.c
index c8e0156974d9e0b6f3ff6fbd6267bd596bd9fb8f..6eee450fe40c120e9b39105a780247ae495f84c7 100644 (file)
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -246,7 +246,7 @@ static QMPRequest *monitor_qmp_dispatcher_pop_any(void)
          *
          * Clear qmp_dispatcher_co_busy before reading request.
          */
-        qatomic_mb_set(&qmp_dispatcher_co_busy, false);
+        qatomic_set_mb(&qmp_dispatcher_co_busy, false);
 
         WITH_QEMU_LOCK_GUARD(&monitor_lock) {
             QMPRequest *req_obj;

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 9cbc8172b5f20d6c55acdc852020143ad8d80781..fed20ffb5dd2e3bec15eb4f4f4a9423fbff5be35 100644 (file)
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -405,7 +405,7 @@ static void qemu_cpu_stop(CPUState *cpu, bool exit)
 
 void qemu_wait_io_event_common(CPUState *cpu)
 {
-    qatomic_mb_set(&cpu->thread_kicked, false);
+    qatomic_set_mb(&cpu->thread_kicked, false);
     if (cpu->stop) {
         qemu_cpu_stop(cpu, false);
     }

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 9d7e172260f1370b9d5d0f1e5bd3bd8bb487b0c3..588d0d166b91259ef974e9b4e88768589761d876 100644 (file)
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3132,7 +3132,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
     bounce.buffer = NULL;
     memory_region_unref(bounce.mr);
     /* Clear in_use before reading map_client_list.  */
-    qatomic_mb_set(&bounce.in_use, false);
+    qatomic_set_mb(&bounce.in_use, false);
     cpu_notify_map_clients();
 }
 

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index ad65603445e5a68909891a5be5aaac31913b21ee..5900dc788f4c22122530ab1dda095a2f8069c620 100644 (file)
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1229,7 +1229,7 @@ static void hvf_wait_for_ipi(CPUState *cpu, struct timespec *ts)
      * Use pselect to sleep so that other threads can IPI us while we're
      * sleeping.
      */
-    qatomic_mb_set(&cpu->thread_kicked, false);
+    qatomic_set_mb(&cpu->thread_kicked, false);
     qemu_mutex_unlock_iothread();
     pselect(0, 0, 0, 0, ts, &cpu->hvf->unblock_ipi_mask);
     qemu_mutex_lock_iothread();

diff --git a/tests/unit/test-aio-multithread.c b/tests/unit/test-aio-multithread.c
index 80c5d4e2e6e378ced0e830bb15cc9966d2184143..08d4570ccb14b522b3fd07da0d49f5ddf6221727 100644 (file)
--- a/tests/unit/test-aio-multithread.c
+++ b/tests/unit/test-aio-multithread.c
@@ -154,7 +154,7 @@ static coroutine_fn void test_multi_co_schedule_entry(void *opaque)
         n = g_test_rand_int_range(0, NUM_CONTEXTS);
         schedule_next(n);
 
-        qatomic_mb_set(&to_schedule[id], qemu_coroutine_self());
+        qatomic_set_mb(&to_schedule[id], qemu_coroutine_self());
         /* finish_cb can run here.  */
         qemu_coroutine_yield();
         g_assert(to_schedule[id] == NULL);

diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 84a50a9e91170b1bd93420ff19c59b51fc3056f2..2534435388f3dce83de34d5f8a7f0594294975d3 100644 (file)
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -202,7 +202,7 @@ static void coroutine_fn qemu_co_mutex_lock_slowpath(AioContext *ctx,
     push_waiter(mutex, &w);
 
     /*
-     * Add waiter before reading mutex->handoff.  Pairs with qatomic_mb_set
+     * Add waiter before reading mutex->handoff.  Pairs with qatomic_set_mb
      * in qemu_co_mutex_unlock.
      */
     smp_mb__after_rmw();
@@ -310,7 +310,7 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
 
         our_handoff = mutex->sequence;
         /* Set handoff before checking for waiters.  */
-        qatomic_mb_set(&mutex->handoff, our_handoff);
+        qatomic_set_mb(&mutex->handoff, our_handoff);
         if (!has_waiters(mutex)) {
             /* The concurrent lock has not added itself yet, so it
              * will be able to pick our handoff.