Re: [Qemu-devel] [PATCH 3/5] qemu-thread: use acquire/release to clarify semantics of QemuEvent

[Paolo Bonzini]

On 12/10/2016 11:21, Alex Bennée wrote:
> 
> 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.

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
>