It seems that aio_wait_kick always required a memory barrier
or atomic operation in the caller, but almost nobody actually
took care of doing it.

Let's put the barrier in the function instead.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 util/aio-wait.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/aio-wait.c b/util/aio-wait.c
index bdb3d3af22..c0a343ac87 100644
--- a/util/aio-wait.c
+++ b/util/aio-wait.c
@@ -35,7 +35,8 @@ static void dummy_bh_cb(void *opaque)
 
 void aio_wait_kick(void)
 {
-    /* The barrier (or an atomic op) is in the caller.  */
+    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

On 4/26/22 10:51, Emanuele Giuseppe Esposito wrote:
> It seems that aio_wait_kick always required a memory barrier
> or atomic operation in the caller, but almost nobody actually
> took care of doing it.

I could not find any barrier in the callers, so I would remove the "almost".

Paolo

On Tue, Apr 26, 2022 at 04:51:07AM -0400, Emanuele Giuseppe Esposito wrote:
> It seems that aio_wait_kick always required a memory barrier
> or atomic operation in the caller, but almost nobody actually
> took care of doing it.
> 
> Let's put the barrier in the function instead.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  util/aio-wait.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/aio-wait.c b/util/aio-wait.c
> index bdb3d3af22..c0a343ac87 100644
> --- a/util/aio-wait.c
> +++ b/util/aio-wait.c
> @@ -35,7 +35,8 @@ static void dummy_bh_cb(void *opaque)
>  
>  void aio_wait_kick(void)
>  {
> -    /* The barrier (or an atomic op) is in the caller.  */
> +    smp_mb();

What is the purpose of the barrier and what does it pair with?

I guess we want to make sure that all stores before aio_wait_kick() are
visible to the other thread's AIO_WAIT_WHILE() cond expression. that
would require smp_wmb(). I'm not sure why it's a full smp_mb() barrier.

> +
>      if (qatomic_read(&global_aio_wait.num_waiters)) {
>          aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
>      }
> -- 
> 2.31.1
> 

Am 28/04/2022 um 13:09 schrieb Stefan Hajnoczi:
> On Tue, Apr 26, 2022 at 04:51:07AM -0400, Emanuele Giuseppe Esposito wrote:
>> It seems that aio_wait_kick always required a memory barrier
>> or atomic operation in the caller, but almost nobody actually
>> took care of doing it.
>>
>> Let's put the barrier in the function instead.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>  util/aio-wait.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/aio-wait.c b/util/aio-wait.c
>> index bdb3d3af22..c0a343ac87 100644
>> --- a/util/aio-wait.c
>> +++ b/util/aio-wait.c
>> @@ -35,7 +35,8 @@ static void dummy_bh_cb(void *opaque)
>>  
>>  void aio_wait_kick(void)
>>  {
>> -    /* The barrier (or an atomic op) is in the caller.  */
>> +    smp_mb();
> 
> What is the purpose of the barrier and what does it pair with?
> 
> I guess we want to make sure that all stores before aio_wait_kick() are
> visible to the other thread's AIO_WAIT_WHILE() cond expression. that
> would require smp_wmb(). I'm not sure why it's a full smp_mb() barrier.

I think we need the full smp_mb barrier because we have a read
afterwards (num_readers) and we want to ensure ordering also for that.

Regarding pairing, yes you are right. I need to also add a smp_mb()
between the write(num_waiters) and read(condition) in AIO_WAIT_WHILE,
otherwise it won't work properly.

So we basically have

Caller:
	write(condition)
	aio_wait_kick()
		smp_mb()
		read(num_writers)

AIO_WAIT_WHILE:
	write(num_writers)
	read(condition)

Emanuele

> 
>> +
>>      if (qatomic_read(&global_aio_wait.num_waiters)) {
>>          aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
>>      }
>> -- 
>> 2.31.1
>>

On Fri, Apr 29, 2022 at 10:06:33AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 28/04/2022 um 13:09 schrieb Stefan Hajnoczi:
> > On Tue, Apr 26, 2022 at 04:51:07AM -0400, Emanuele Giuseppe Esposito wrote:
> >> It seems that aio_wait_kick always required a memory barrier
> >> or atomic operation in the caller, but almost nobody actually
> >> took care of doing it.
> >>
> >> Let's put the barrier in the function instead.
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >> ---
> >>  util/aio-wait.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/util/aio-wait.c b/util/aio-wait.c
> >> index bdb3d3af22..c0a343ac87 100644
> >> --- a/util/aio-wait.c
> >> +++ b/util/aio-wait.c
> >> @@ -35,7 +35,8 @@ static void dummy_bh_cb(void *opaque)
> >>  
> >>  void aio_wait_kick(void)
> >>  {
> >> -    /* The barrier (or an atomic op) is in the caller.  */
> >> +    smp_mb();
> > 
> > What is the purpose of the barrier and what does it pair with?
> > 
> > I guess we want to make sure that all stores before aio_wait_kick() are
> > visible to the other thread's AIO_WAIT_WHILE() cond expression. that
> > would require smp_wmb(). I'm not sure why it's a full smp_mb() barrier.
> 
> I think we need the full smp_mb barrier because we have a read
> afterwards (num_readers) and we want to ensure ordering also for that.
> 
> Regarding pairing, yes you are right. I need to also add a smp_mb()
> between the write(num_waiters) and read(condition) in AIO_WAIT_WHILE,
> otherwise it won't work properly.
> 
> So we basically have
> 
> Caller:
> 	write(condition)
> 	aio_wait_kick()
> 		smp_mb()
> 		read(num_writers)
> 
> AIO_WAIT_WHILE:
> 	write(num_writers)
> 	read(condition)

That makes sense to me, thank you! Please include the explanation in the
comments.

Stefan