From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42820) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buFjM-0002ax-5s for qemu-devel@nongnu.org; Wed, 12 Oct 2016 05:21:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buFjI-0005fn-1Q for qemu-devel@nongnu.org; Wed, 12 Oct 2016 05:21:43 -0400 Received: from mail-wm0-x236.google.com ([2a00:1450:400c:c09::236]:34462) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buFjH-0005fG-N2 for qemu-devel@nongnu.org; Wed, 12 Oct 2016 05:21:39 -0400 Received: by mail-wm0-x236.google.com with SMTP id c78so2676400wme.1 for ; Wed, 12 Oct 2016 02:21:39 -0700 (PDT) References: <1476107947-31430-1-git-send-email-pbonzini@redhat.com> <1476107947-31430-4-git-send-email-pbonzini@redhat.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1476107947-31430-4-git-send-email-pbonzini@redhat.com> Date: Wed, 12 Oct 2016 10:21:36 +0100 Message-ID: <87y41torbz.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 3/5] qemu-thread: use acquire/release to clarify semantics of QemuEvent List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, cota@braap.org Paolo Bonzini 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 > --- > 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