| Message ID | 20200805100051.361547-3-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | aio-posix: keep aio_notify_me disabled during polling | expand |
On 05/08/20 12:00, Stefan Hajnoczi wrote: > aio_notify() does not set ctx->notified when called with > ctx->aio_notify_me disabled. Therefore aio_notify_me needs to be enabled > during polling. > > This is suboptimal since expensive event_notifier_set(&ctx->notifier) > and event_notifier_test_and_clear(&ctx->notifier) calls are required > when ctx->aio_notify_me is enabled. > > Change aio_notify() so that aio->notified is always set, regardless of > ctx->aio_notify_me. This will make polling cheaper since > ctx->aio_notify_me can remain disabled. Move the > event_notifier_test_and_clear() to the fd handler function (which is now > no longer an empty function so "dummy" has been dropped from its name). > > The next patch takes advantage of this by optimizing polling in > util/aio-posix.c. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/async.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/util/async.c b/util/async.c > index d9f987e133..3ec3e8d135 100644 > --- a/util/async.c > +++ b/util/async.c > @@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx) > > void aio_notify(AioContext *ctx) > { > - /* Write e.g. bh->scheduled before reading ctx->notify_me. Pairs > + /* > + * Write e.g. bh->flags before writing ctx->notified. Pairs with smp_mb in > + * aio_notify_accept. > + */ > + smp_wmb(); > + atomic_set(&ctx->notified, true); > + > + /* > + * Write ctx->notified before reading ctx->notify_me. Pairs > * with smp_mb in aio_ctx_prepare or aio_poll. > */ > smp_mb(); > if (atomic_read(&ctx->notify_me)) { > event_notifier_set(&ctx->notifier); > - atomic_mb_set(&ctx->notified, true); > } > } > > void aio_notify_accept(AioContext *ctx) > { > - if (atomic_xchg(&ctx->notified, false) > -#ifdef WIN32 > - || true > -#endif > - ) { > - event_notifier_test_and_clear(&ctx->notifier); > - } > + atomic_set(&ctx->notified, false); > + > + /* > + * Write ctx->notified before reading e.g. bh->flags. Pairs with smp_mb in > + * aio_notify. Just a nit: it's an smp_wmb(). (It's okay for a wmb to pair with anything stronger than a smp_rmb()). > + */ > + smp_mb(); > } > > static void aio_timerlist_notify(void *opaque, QEMUClockType type) > @@ -445,8 +452,11 @@ static void aio_timerlist_notify(void *opaque, QEMUClockType type) > aio_notify(opaque); > } > > -static void aio_context_notifier_dummy_cb(EventNotifier *e) > +static void aio_context_notifier_cb(EventNotifier *e) > { > + AioContext *ctx = container_of(e, AioContext, notifier); > + > + event_notifier_test_and_clear(&ctx->notifier); > } > > /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */ > @@ -508,7 +518,7 @@ AioContext *aio_context_new(Error **errp) > > aio_set_event_notifier(ctx, &ctx->notifier, > false, > - aio_context_notifier_dummy_cb, > + aio_context_notifier_cb, > aio_context_notifier_poll); > #ifdef CONFIG_LINUX_AIO > ctx->linux_aio = NULL; > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Much better, you can almost trust the code to do the right thing. :) Paolo
diff --git a/util/async.c b/util/async.c index d9f987e133..3ec3e8d135 100644 --- a/util/async.c +++ b/util/async.c @@ -419,25 +419,32 @@ LuringState *aio_get_linux_io_uring(AioContext *ctx) void aio_notify(AioContext *ctx) { - /* Write e.g. bh->scheduled before reading ctx->notify_me. Pairs + /* + * Write e.g. bh->flags before writing ctx->notified. Pairs with smp_mb in + * aio_notify_accept. + */ + smp_wmb(); + atomic_set(&ctx->notified, true); + + /* + * Write ctx->notified before reading ctx->notify_me. Pairs * with smp_mb in aio_ctx_prepare or aio_poll. */ smp_mb(); if (atomic_read(&ctx->notify_me)) { event_notifier_set(&ctx->notifier); - atomic_mb_set(&ctx->notified, true); } } void aio_notify_accept(AioContext *ctx) { - if (atomic_xchg(&ctx->notified, false) -#ifdef WIN32 - || true -#endif - ) { - event_notifier_test_and_clear(&ctx->notifier); - } + atomic_set(&ctx->notified, false); + + /* + * Write ctx->notified before reading e.g. bh->flags. Pairs with smp_mb in + * aio_notify. + */ + smp_mb(); } static void aio_timerlist_notify(void *opaque, QEMUClockType type) @@ -445,8 +452,11 @@ static void aio_timerlist_notify(void *opaque, QEMUClockType type) aio_notify(opaque); } -static void aio_context_notifier_dummy_cb(EventNotifier *e) +static void aio_context_notifier_cb(EventNotifier *e) { + AioContext *ctx = container_of(e, AioContext, notifier); + + event_notifier_test_and_clear(&ctx->notifier); } /* Returns true if aio_notify() was called (e.g. a BH was scheduled) */ @@ -508,7 +518,7 @@ AioContext *aio_context_new(Error **errp) aio_set_event_notifier(ctx, &ctx->notifier, false, - aio_context_notifier_dummy_cb, + aio_context_notifier_cb, aio_context_notifier_poll); #ifdef CONFIG_LINUX_AIO ctx->linux_aio = NULL;
aio_notify() does not set ctx->notified when called with ctx->aio_notify_me disabled. Therefore aio_notify_me needs to be enabled during polling. This is suboptimal since expensive event_notifier_set(&ctx->notifier) and event_notifier_test_and_clear(&ctx->notifier) calls are required when ctx->aio_notify_me is enabled. Change aio_notify() so that aio->notified is always set, regardless of ctx->aio_notify_me. This will make polling cheaper since ctx->aio_notify_me can remain disabled. Move the event_notifier_test_and_clear() to the fd handler function (which is now no longer an empty function so "dummy" has been dropped from its name). The next patch takes advantage of this by optimizing polling in util/aio-posix.c. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- util/async.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)