[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/5] qemu-thread: use acquire/release to clarify
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH 3/5] qemu-thread: use acquire/release to clarify semantics of QemuEvent |
Date: |
Wed, 12 Oct 2016 10:21:36 +0100 |
User-agent: |
mu4e 0.9.17; emacs 25.1.50.7 |
Paolo Bonzini <address@hidden> writes:
> 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..dcdc014 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 (atomic_read(&ev->value) == EV_SET) {
> /* If there was a concurrent reset (or even reset+wait),
> * do nothing. Otherwise change EV_SET->EV_FREE.
Why are we saving value here? We never use it.
> */
> @@ -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
--
Alex Bennée
- [Qemu-devel] [PATCH 0/5] More thread sanitizer fixes and atomic.h improvements, Paolo Bonzini, 2016/10/10
- [Qemu-devel] [PATCH 2/5] cpus: use atomic_read to read seqlock-protected variables, Paolo Bonzini, 2016/10/10
- [Qemu-devel] [PATCH 3/5] qemu-thread: use acquire/release to clarify semantics of QemuEvent, Paolo Bonzini, 2016/10/10
- Re: [Qemu-devel] [PATCH 3/5] qemu-thread: use acquire/release to clarify semantics of QemuEvent,
Alex Bennée <=
- [Qemu-devel] [PATCH 1/5] atomic: introduce smp_mb_acquire and smp_mb_release, Paolo Bonzini, 2016/10/10
- [Qemu-devel] [PATCH 4/5] rcu: simplify memory barriers, Paolo Bonzini, 2016/10/10
- [Qemu-devel] [PATCH 5/5] atomic: base mb_read/mb_set on load-acquire and store-release, Paolo Bonzini, 2016/10/10
- Re: [Qemu-devel] [PATCH 0/5] More thread sanitizer fixes and atomic.h improvements, no-reply, 2016/10/10