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 <stefa...@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 <pbonz...@redhat.com>
Much better, you can almost trust the code to do the right thing. :)
Paolo
