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 <hre...@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 <stefa...@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
>
signature.asc
Description: PGP signature
