[Qemu-devel] [PULL 07/50] qemu-thread: use acquire/release to clarify semantics of QemuEvent

[Paolo Bonzini]

Do not use the somewhat mysterious atomic_mb_read/atomic_mb_set,
instead make sure that the operations on QemuEvent are annotated
with the desired acquire and release semantics.

In particular, qemu_event_set wakes up the waiting thread, so it must
be a release from the POV of the waker (compare with qemu_mutex_unlock).
And it actually needs a full barrier, because that's the only thing that
provides something like a "load-release".

Use smp_mb_acquire until we have atomic_load_acquire and
atomic_store_release in atomic.h.

Signed-off-by: Paolo Bonzini <address@hidden>
---
 util/qemu-thread-posix.c | 15 ++++++++++++---
 util/qemu-thread-win32.c | 15 ++++++++++++---
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 74a3023..ce51b37 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -360,7 +360,11 @@ void qemu_event_destroy(QemuEvent *ev)
 
 void qemu_event_set(QemuEvent *ev)
 {
-    if (atomic_mb_read(&ev->value) != EV_SET) {
+    /* qemu_event_set has release semantics, but because it *loads*
+     * ev->value we need a full memory barrier here.
+     */
+    smp_mb();
+    if (atomic_read(&ev->value) != EV_SET) {
         if (atomic_xchg(&ev->value, EV_SET) == EV_BUSY) {
             /* There were waiters, wake them up.  */
             futex_wake(ev, INT_MAX);
@@ -370,7 +374,11 @@ void qemu_event_set(QemuEvent *ev)
 
 void qemu_event_reset(QemuEvent *ev)
 {
-    if (atomic_mb_read(&ev->value) == EV_SET) {
+    unsigned value;
+
+    value = atomic_read(&ev->value);
+    smp_mb_acquire();
+    if (value == EV_SET) {
         /*
          * If there was a concurrent reset (or even reset+wait),
          * do nothing.  Otherwise change EV_SET->EV_FREE.
@@ -383,7 +391,8 @@ void qemu_event_wait(QemuEvent *ev)
 {
     unsigned value;
 
-    value = atomic_mb_read(&ev->value);
+    value = atomic_read(&ev->value);
+    smp_mb_acquire();
     if (value != EV_SET) {
         if (value == EV_FREE) {
             /*
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 98a5ddf..072806f 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -274,7 +274,11 @@ void qemu_event_destroy(QemuEvent *ev)
 
 void qemu_event_set(QemuEvent *ev)
 {
-    if (atomic_mb_read(&ev->value) != EV_SET) {
+    /* qemu_event_set has release semantics, but because it *loads*
+     * ev->value we need a full memory barrier here.
+     */
+    smp_mb();
+    if (atomic_read(&ev->value) != EV_SET) {
         if (atomic_xchg(&ev->value, EV_SET) == EV_BUSY) {
             /* There were waiters, wake them up.  */
             SetEvent(ev->event);
@@ -284,7 +288,11 @@ void qemu_event_set(QemuEvent *ev)
 
 void qemu_event_reset(QemuEvent *ev)
 {
-    if (atomic_mb_read(&ev->value) == EV_SET) {
+    unsigned value;
+
+    value = atomic_read(&ev->value);
+    smp_mb_acquire();
+    if (value == EV_SET) {
         /* If there was a concurrent reset (or even reset+wait),
          * do nothing.  Otherwise change EV_SET->EV_FREE.
          */
@@ -296,7 +304,8 @@ void qemu_event_wait(QemuEvent *ev)
 {
     unsigned value;
 
-    value = atomic_mb_read(&ev->value);
+    value = atomic_read(&ev->value);
+    smp_mb_acquire();
     if (value != EV_SET) {
         if (value == EV_FREE) {
             /* qemu_event_set is not yet going to call SetEvent, but we are
-- 
1.8.3.1