Re: [PATCH v2 2/3] async: always set ctx->notified in aio_notify()

[Paolo Bonzini <pbonzini@redhat.com>]

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