util/qemu-coroutine.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
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
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
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 > >
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
© 2016 - 2024 Red Hat, Inc.