| Message ID | 1476107947-31430-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com> > --- > 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
On 12/10/2016 11:21, Alex Bennée wrote: > > Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com> >> --- >> 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. It should be used in the "if", and patchew rightly complained. I didn't run the docker-test-mingw compilation on this series. Paolo > >> */ >> @@ -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 >
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. */ @@ -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
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 <pbonzini@redhat.com> --- util/qemu-thread-posix.c | 15 ++++++++++++--- util/qemu-thread-win32.c | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-)