Re: [PATCH 2/2] virtio: Keep notifications disabled during drain

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6083 bytes --]

On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote:
> During drain, we do not care about virtqueue notifications, which is why
> we remove the handlers on it.  When removing those handlers, whether vq
> notifications are enabled or not depends on whether we were in polling
> mode or not; if not, they are enabled (by default); if so, they have
> been disabled by the io_poll_start callback.
> 
> Because we do not care about those notifications after removing the
> handlers, this is fine.  However, we have to explicitly ensure they are
> enabled when re-attaching the handlers, so we will resume receiving
> notifications.  We do this in virtio_queue_aio_attach_host_notifier*().
> If such a function is called while we are in a polling section,
> attaching the notifiers will then invoke the io_poll_start callback,
> re-disabling notifications.
> 
> Because we will always miss virtqueue updates in the drained section, we
> also need to poll the virtqueue once after attaching the notifiers.
> 
> Buglink: https://issues.redhat.com/browse/RHEL-3934
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  include/block/aio.h |  7 ++++++-
>  hw/virtio/virtio.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 5d0a114988..8378553eb9 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -480,9 +480,14 @@ void aio_set_event_notifier(AioContext *ctx,
>                              AioPollFn *io_poll,
>                              EventNotifierHandler *io_poll_ready);
>  
> -/* Set polling begin/end callbacks for an event notifier that has already been
> +/*
> + * Set polling begin/end callbacks for an event notifier that has already been
>   * registered with aio_set_event_notifier.  Do nothing if the event notifier is
>   * not registered.
> + *
> + * Note that if the io_poll_end() callback (or the entire notifier) is removed
> + * during polling, it will not be called, so an io_poll_begin() is not
> + * necessarily always followed by an io_poll_end().
>   */
>  void aio_set_event_notifier_poll(AioContext *ctx,
>                                   EventNotifier *notifier,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7549094154..4166da9e97 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3556,6 +3556,17 @@ static void virtio_queue_host_notifier_aio_poll_end(EventNotifier *n)
>  
>  void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
>  {
> +    /*
> +     * virtio_queue_aio_detach_host_notifier() can leave notifications disabled.
> +     * Re-enable them.  (And if detach has not been used before, notifications
> +     * being enabled is still the default state while a notifier is attached;
> +     * see virtio_queue_host_notifier_aio_poll_end(), which will always leave
> +     * notifications enabled once the polling section is left.)
> +     */
> +    if (!virtio_queue_get_notification(vq)) {
> +        virtio_queue_set_notification(vq, 1);
> +    }
> +
>      aio_set_event_notifier(ctx, &vq->host_notifier,
>                             virtio_queue_host_notifier_read,
>                             virtio_queue_host_notifier_aio_poll,
> @@ -3563,6 +3574,13 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
>      aio_set_event_notifier_poll(ctx, &vq->host_notifier,
>                                  virtio_queue_host_notifier_aio_poll_begin,
>                                  virtio_queue_host_notifier_aio_poll_end);
> +
> +    /*
> +     * We will have ignored notifications about new requests from the guest
> +     * during the drain, so "kick" the virt queue to process those requests
> +     * now.
> +     */
> +    virtio_queue_notify(vq->vdev, vq->queue_index);

event_notifier_set(&vq->host_notifier) is easier to understand because
it doesn't contain a non-host_notifier code path that we must not take.

Is there a reason why you used virtio_queue_notify() instead?

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

>  }
>  
>  /*
> @@ -3573,14 +3591,38 @@ void virtio_queue_aio_attach_host_notifier(VirtQueue *vq, AioContext *ctx)
>   */
>  void virtio_queue_aio_attach_host_notifier_no_poll(VirtQueue *vq, AioContext *ctx)
>  {
> +    /* See virtio_queue_aio_attach_host_notifier() */
> +    if (!virtio_queue_get_notification(vq)) {
> +        virtio_queue_set_notification(vq, 1);
> +    }
> +
>      aio_set_event_notifier(ctx, &vq->host_notifier,
>                             virtio_queue_host_notifier_read,
>                             NULL, NULL);
> +
> +    /*
> +     * See virtio_queue_aio_attach_host_notifier().
> +     * Note that this may be unnecessary for the type of virtqueues this
> +     * function is used for.  Still, it will not hurt to have a quick look into
> +     * whether we can/should process any of the virtqueue elements.
> +     */
> +    virtio_queue_notify(vq->vdev, vq->queue_index);
>  }
>  
>  void virtio_queue_aio_detach_host_notifier(VirtQueue *vq, AioContext *ctx)
>  {
>      aio_set_event_notifier(ctx, &vq->host_notifier, NULL, NULL, NULL);
> +
> +    /*
> +     * aio_set_event_notifier_poll() does not guarantee whether io_poll_end()
> +     * will run after io_poll_begin(), so by removing the notifier, we do not
> +     * know whether virtio_queue_host_notifier_aio_poll_end() has run after a
> +     * previous virtio_queue_host_notifier_aio_poll_begin(), i.e. whether
> +     * notifications are enabled or disabled.  It does not really matter anyway;
> +     * we just removed the notifier, so we do not care about notifications until
> +     * we potentially re-attach it.  The attach_host_notifier functions will
> +     * ensure that notifications are enabled again when they are needed.
> +     */
>  }
>  
>  void virtio_queue_host_notifier_read(EventNotifier *n)
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]