From 374293ca6fb060f0302e5ca76ddccbc2bbb075c9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 19 Sep 2016 11:10:57 +0200 Subject: qemu-thread: use acquire/release to clarify semantics of QemuEvent 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 --- util/qemu-thread-posix.c | 15 ++++++++++++--- util/qemu-thread-win32.c | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) (limited to 'util') 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 -- cgit v1.1 From e11131b02584ddea93deef1caad8e4f945dd7340 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 19 Sep 2016 11:27:46 +0200 Subject: rcu: simplify memory barriers Thanks to the acquire semantics of qemu_event_reset and qemu_event_wait, some memory barriers can be removed. Signed-off-by: Paolo Bonzini --- util/rcu.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'util') diff --git a/util/rcu.c b/util/rcu.c index bceb3e4..9adc5e4 100644 --- a/util/rcu.c +++ b/util/rcu.c @@ -82,14 +82,16 @@ static void wait_for_readers(void) /* Instead of using atomic_mb_set for index->waiting, and * atomic_mb_read for index->ctr, memory barriers are placed * manually since writes to different threads are independent. - * atomic_mb_set has a smp_wmb before... + * qemu_event_reset has acquire semantics, so no memory barrier + * is needed here. */ - smp_wmb(); QLIST_FOREACH(index, ®istry, node) { atomic_set(&index->waiting, true); } - /* ... and a smp_mb after. */ + /* Here, order the stores to index->waiting before the + * loads of index->ctr. + */ smp_mb(); QLIST_FOREACH_SAFE(index, ®istry, node, tmp) { @@ -104,9 +106,6 @@ static void wait_for_readers(void) } } - /* atomic_mb_read has smp_rmb after. */ - smp_rmb(); - if (QLIST_EMPTY(®istry)) { break; } -- cgit v1.1