Re: [RFC PATCH v2 1/8] aio_wait_kick: add missing memory barrier

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature