[PATCH] aio_wait_kick: add missing memory barrier

Emanuele Giuseppe Esposito posted 1 patch 1 month ago
include/block/aio-wait.h |  2 ++
util/aio-wait.c          | 16 +++++++++++++++-
2 files changed, 17 insertions(+), 1 deletion(-)
[PATCH] aio_wait_kick: add missing memory barrier
Posted by Emanuele Giuseppe Esposito 1 month ago
It seems that aio_wait_kick always required a memory barrier
or atomic operation in the caller, but nobody actually
took care of doing it.

Let's put the barrier in the function instead, and pair it
with another one in AIO_WAIT_WHILE. Read aio_wait_kick()
comment for further explanation.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/block/aio-wait.h |  2 ++
 util/aio-wait.c          | 16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index b39eefb38d..54840f8622 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -81,6 +81,8 @@ extern AioWait global_aio_wait;
     AioContext *ctx_ = (ctx);                                      \
     /* Increment wait_->num_waiters before evaluating cond. */     \
     qatomic_inc(&wait_->num_waiters);                              \
+    /* Paired with smp_mb in aio_wait_kick(). */                   \
+    smp_mb();                                                      \
     if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
         while ((cond)) {                                           \
             aio_poll(ctx_, true);                                  \
diff --git a/util/aio-wait.c b/util/aio-wait.c
index bdb3d3af22..98c5accd29 100644
--- a/util/aio-wait.c
+++ b/util/aio-wait.c
@@ -35,7 +35,21 @@ static void dummy_bh_cb(void *opaque)
 
 void aio_wait_kick(void)
 {
-    /* The barrier (or an atomic op) is in the caller.  */
+    /*
+     * Paired with smp_mb in AIO_WAIT_WHILE. Here we have:
+     * write(condition);
+     * aio_wait_kick() {
+     *      smp_mb();
+     *      read(num_waiters);
+     * }
+     *
+     * And in AIO_WAIT_WHILE:
+     * write(num_waiters);
+     * smp_mb();
+     * read(condition);
+     */
+    smp_mb();
+
     if (qatomic_read(&global_aio_wait.num_waiters)) {
         aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
     }
-- 
2.31.1
Re: [PATCH] aio_wait_kick: add missing memory barrier
Posted by Roman Kagan 3 weeks ago
On Tue, May 24, 2022 at 01:30:54PM -0400, Emanuele Giuseppe Esposito wrote:
> It seems that aio_wait_kick always required a memory barrier
> or atomic operation in the caller, but nobody actually
> took care of doing it.
> 
> Let's put the barrier in the function instead, and pair it
> with another one in AIO_WAIT_WHILE. Read aio_wait_kick()
> comment for further explanation.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  include/block/aio-wait.h |  2 ++
>  util/aio-wait.c          | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> index b39eefb38d..54840f8622 100644
> --- a/include/block/aio-wait.h
> +++ b/include/block/aio-wait.h
> @@ -81,6 +81,8 @@ extern AioWait global_aio_wait;
>      AioContext *ctx_ = (ctx);                                      \
>      /* Increment wait_->num_waiters before evaluating cond. */     \
>      qatomic_inc(&wait_->num_waiters);                              \
> +    /* Paired with smp_mb in aio_wait_kick(). */                   \
> +    smp_mb();                                                      \

IIRC qatomic_inc() ensures sequential consistency, isn't it enough here?

>      if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>          while ((cond)) {                                           \
>              aio_poll(ctx_, true);                                  \

Roman.
Re: [PATCH] aio_wait_kick: add missing memory barrier
Posted by Paolo Bonzini 2 weeks, 6 days ago
On 6/4/22 14:51, Roman Kagan wrote:
> On Tue, May 24, 2022 at 01:30:54PM -0400, Emanuele Giuseppe Esposito wrote:
>> It seems that aio_wait_kick always required a memory barrier
>> or atomic operation in the caller, but nobody actually
>> took care of doing it.
>>
>> Let's put the barrier in the function instead, and pair it
>> with another one in AIO_WAIT_WHILE. Read aio_wait_kick()
>> comment for further explanation.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   include/block/aio-wait.h |  2 ++
>>   util/aio-wait.c          | 16 +++++++++++++++-
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
>> index b39eefb38d..54840f8622 100644
>> --- a/include/block/aio-wait.h
>> +++ b/include/block/aio-wait.h
>> @@ -81,6 +81,8 @@ extern AioWait global_aio_wait;
>>       AioContext *ctx_ = (ctx);                                      \
>>       /* Increment wait_->num_waiters before evaluating cond. */     \
>>       qatomic_inc(&wait_->num_waiters);                              \
>> +    /* Paired with smp_mb in aio_wait_kick(). */                   \
>> +    smp_mb();                                                      \
> 
> IIRC qatomic_inc() ensures sequential consistency, isn't it enough here?

Nope, it only ensures sequential consistency with other SEQ_CST 
operations, i.e. not with qatomic_read or qatomic_set. :(

The smp_mb() is needed on ARM, in particular.


Paolo
Re: [PATCH] aio_wait_kick: add missing memory barrier
Posted by Kevin Wolf 3 weeks, 5 days ago
Am 24.05.2022 um 19:30 hat Emanuele Giuseppe Esposito geschrieben:
> It seems that aio_wait_kick always required a memory barrier
> or atomic operation in the caller, but nobody actually
> took care of doing it.
> 
> Let's put the barrier in the function instead, and pair it
> with another one in AIO_WAIT_WHILE. Read aio_wait_kick()
> comment for further explanation.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Thanks, applied to the block branch.

Kevin
Re: [PATCH] aio_wait_kick: add missing memory barrier
Posted by Stefan Hajnoczi 1 month ago
On Tue, May 24, 2022 at 01:30:54PM -0400, Emanuele Giuseppe Esposito wrote:
> It seems that aio_wait_kick always required a memory barrier
> or atomic operation in the caller, but nobody actually
> took care of doing it.
> 
> Let's put the barrier in the function instead, and pair it
> with another one in AIO_WAIT_WHILE. Read aio_wait_kick()
> comment for further explanation.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  include/block/aio-wait.h |  2 ++
>  util/aio-wait.c          | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [PATCH] aio_wait_kick: add missing memory barrier
Posted by Vladimir Sementsov-Ogievskiy 1 month ago
On 5/24/22 20:30, Emanuele Giuseppe Esposito wrote:
> It seems that aio_wait_kick always required a memory barrier
> or atomic operation in the caller, but nobody actually
> took care of doing it.
> 
> Let's put the barrier in the function instead, and pair it
> with another one in AIO_WAIT_WHILE. Read aio_wait_kick()
> comment for further explanation.
> 
> Suggested-by: Paolo Bonzini<pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>

Thanks!

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir