On Fri, Apr 07, 2023 at 10:32:39AM +0200, Paolo Bonzini wrote: > Il gio 6 apr 2023, 12:55 Stefan Hajnoczi <stefa...@gmail.com> ha scritto: > > > On Thu, 6 Apr 2023 at 06:09, Paolo Bonzini <pbonz...@redhat.com> wrote: > > > > > > Replace with an explicit barrier and a comment. > > > > > > Signed-off-by: Paolo Bonzini <pbonz...@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 <stefa...@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
signature.asc
Description: PGP signature
