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

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

[Paolo Bonzini]

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();
+
+        const char *scheduled = qatomic_read(&to->scheduled);
 
         QSIMPLEQ_REMOVE_HEAD(&pending, co_queue_next);
 
-- 
2.39.2

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

[Stefan Hajnoczi]

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.

Does smp_read_barrier_depends() make sense for QEMU? Does QEMU support
Alpha host CPUs?

Stefan

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

[Paolo Bonzini]

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

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.

Paolo


> Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 3222 bytes --]

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

[Stefan Hajnoczi]

[-- 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 --]