mutex->from_push and mutex->handoff in qemu-coroutine-lock implement
the familiar pattern:

   write a                                  write b
   smp_mb()                                 smp_mb()
   read b                                   read a

The memory barrier is required by the C memory model even after a
SEQ_CST read-modify-write operation such as QSLIST_INSERT_HEAD_ATOMIC.
Add it and avoid the unclear qatomic_mb_read() operation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-coroutine-lock.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 58f3f771817b..84a50a9e9117 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -201,10 +201,16 @@ static void coroutine_fn qemu_co_mutex_lock_slowpath(AioContext *ctx,
     trace_qemu_co_mutex_lock_entry(mutex, self);
     push_waiter(mutex, &w);
 
+    /*
+     * Add waiter before reading mutex->handoff.  Pairs with qatomic_mb_set
+     * in qemu_co_mutex_unlock.
+     */
+    smp_mb__after_rmw();
+
     /* This is the "Responsibility Hand-Off" protocol; a lock() picks from
      * a concurrent unlock() the responsibility of waking somebody up.
      */
-    old_handoff = qatomic_mb_read(&mutex->handoff);
+    old_handoff = qatomic_read(&mutex->handoff);
     if (old_handoff &&
         has_waiters(mutex) &&
         qatomic_cmpxchg(&mutex->handoff, old_handoff, 0) == old_handoff) {
@@ -303,6 +309,7 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
         }
 
         our_handoff = mutex->sequence;
+        /* Set handoff before checking for waiters.  */
         qatomic_mb_set(&mutex->handoff, our_handoff);
         if (!has_waiters(mutex)) {
             /* The concurrent lock has not added itself yet, so it
-- 
2.39.1

On 03.03.23 18:19, Paolo Bonzini wrote:
> mutex->from_push and mutex->handoff in qemu-coroutine-lock implement
> the familiar pattern:
> 
>     write a                                  write b
>     smp_mb()                                 smp_mb()
>     read b                                   read a
> 
> The memory barrier is required by the C memory model even after a
> SEQ_CST read-modify-write operation such as QSLIST_INSERT_HEAD_ATOMIC.
> Add it and avoid the unclear qatomic_mb_read() operation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   util/qemu-coroutine-lock.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
> index 58f3f771817b..84a50a9e9117 100644
> --- a/util/qemu-coroutine-lock.c
> +++ b/util/qemu-coroutine-lock.c
> @@ -201,10 +201,16 @@ static void coroutine_fn qemu_co_mutex_lock_slowpath(AioContext *ctx,
>       trace_qemu_co_mutex_lock_entry(mutex, self);
>       push_waiter(mutex, &w);
>   
> +    /*
> +     * Add waiter before reading mutex->handoff.  Pairs with qatomic_mb_set
> +     * in qemu_co_mutex_unlock.
> +     */
> +    smp_mb__after_rmw();
> +
>       /* This is the "Responsibility Hand-Off" protocol; a lock() picks from
>        * a concurrent unlock() the responsibility of waking somebody up.
>        */
> -    old_handoff = qatomic_mb_read(&mutex->handoff);
> +    old_handoff = qatomic_read(&mutex->handoff);
>       if (old_handoff &&
>           has_waiters(mutex) &&
>           qatomic_cmpxchg(&mutex->handoff, old_handoff, 0) == old_handoff) {
> @@ -303,6 +309,7 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
>           }
>   
>           our_handoff = mutex->sequence;
> +        /* Set handoff before checking for waiters.  */
>           qatomic_mb_set(&mutex->handoff, our_handoff);
>           if (!has_waiters(mutex)) {
>               /* The concurrent lock has not added itself yet, so it

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

On 3/3/23 09:19, Paolo Bonzini wrote:
> mutex->from_push and mutex->handoff in qemu-coroutine-lock implement
> the familiar pattern:
> 
>     write a                                  write b
>     smp_mb()                                 smp_mb()
>     read b                                   read a
> 
> The memory barrier is required by the C memory model even after a
> SEQ_CST read-modify-write operation such as QSLIST_INSERT_HEAD_ATOMIC.
> Add it and avoid the unclear qatomic_mb_read() operation.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   util/qemu-coroutine-lock.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~