Re: [PATCH 7/8] qemu-coroutine-lock: add smp_mb__after_rmw()

[David Hildenbrand <david@redhat.com>]

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