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

[Hanna Czenczek]

On 25.01.24 19:03, Stefan Hajnoczi wrote:
> 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?

Not a good one anyway!

virtio_queue_notify() is just what seemed obvious to me (i.e. to notify 
the virtqueue).  Before removal of the AioContext lock, calling 
handle_output seemed safe.  But, yes, there was the discussion on the 
RFC that it really isn’t.  I didn’t consider that means we must rely on 
virtio_queue_notify() calling event_notifier_set(), so we may as well 
call it explicitly here.

I’ll fix it, thanks for pointing it out!

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

Thanks!

Hanna