| Message ID | 20230303171939.237819-8-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix missing memory barriers on ARM | expand |
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~
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>
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
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(-)