Re: [PATCH for-8.1] qemu-coroutine: remove qatomic_mb_read()

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]

On Fri, Apr 07, 2023 at 10:32:39AM +0200, Paolo Bonzini wrote:
> Il gio 6 apr 2023, 12:55 Stefan Hajnoczi <stefanha@gmail.com> ha scritto:
> 
> > On Thu, 6 Apr 2023 at 06:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > Replace with an explicit barrier and a comment.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  util/qemu-coroutine.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> > > index 849452369201..17a88f65053e 100644
> > > --- a/util/qemu-coroutine.c
> > > +++ b/util/qemu-coroutine.c
> > > @@ -127,9 +127,13 @@ void qemu_aio_coroutine_enter(AioContext *ctx,
> > Coroutine *co)
> > >          Coroutine *to = QSIMPLEQ_FIRST(&pending);
> > >          CoroutineAction ret;
> > >
> > > -        /* Cannot rely on the read barrier for to in aio_co_wake(), as
> > there are
> > > -         * callers outside of aio_co_wake() */
> > > -        const char *scheduled = qatomic_mb_read(&to->scheduled);
> > > +        /*
> > > +         * Read to before to->scheduled; pairs with qatomic_cmpxchg in
> > > +         * qemu_co_sleep(), aio_co_schedule() etc.
> > > +         */
> > > +        smp_read_barrier_depends();
> >
> > I'm not a fan of nuanced memory ordering primitives. I don't
> > understand or remember all the primitives available in
> > docs/devel/atomics.rst and especially not how they interact with each
> > other.
> >
> 
> Understood, that's why I want to remove qatomic_mb_read().
> 
> Does smp_read_barrier_depends() make sense for QEMU? Does QEMU support
> > Alpha host CPUs?
> >
> 
> It makes sense in that it's cheaper than qatomic_load_acquire() or
> smp_rmb() on ARM and PPC (32-bit ARM is especially bad). Here I can use
> smp_rmb() if you prefer; I thought that the comment, explicitly referring
> to "to->scheduled" which depends on "to", would be enough.
> 
> I could also use QSIMPLEQ_FIRST_RCU(&pending) to hide the barrier, but it
> seems to be a bad idea because there's no RCU involvement here.

If smp_read_barrier_depends() is cheaper on ARM and PPC than
qatomic_load_acquire() or smp_rmb(), then this seems like a good use of
it:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I didn't know that smp_read_barrier_depends() is relevant on any
architecture other than Alpha. It would be nice if atomics.rst mentioned
ARM and PPC rather than Alpha.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]