| Message ID | 20240124173834.66320-3-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: Keep notifications disabled during drain | expand |
Am 24.01.24 um 18:38 schrieb Hanna Czenczek: > 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> Tested-by: Fiona Ebner <f.ebner@proxmox.com> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 >
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
On 25.01.24 19:18, Hanna Czenczek wrote: > On 25.01.24 19:03, Stefan Hajnoczi wrote: >> On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote: [...] >>> @@ -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! (I think together with this change, I’ll also remove the event_notifier_set() call from virtio_blk_data_plane_start(). It’d obviously be a duplicate, and removing it shows why virtio_queue_aio_attach_host_notifier() should always kick the queue.)
On Thu, Jan 25, 2024 at 07:32:12PM +0100, Hanna Czenczek wrote: > On 25.01.24 19:18, Hanna Czenczek wrote: > > On 25.01.24 19:03, Stefan Hajnoczi wrote: > > > On Wed, Jan 24, 2024 at 06:38:30PM +0100, Hanna Czenczek wrote: > > [...] > > > > > @@ -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! > > (I think together with this change, I’ll also remove the > event_notifier_set() call from virtio_blk_data_plane_start(). It’d > obviously be a duplicate, and removing it shows why > virtio_queue_aio_attach_host_notifier() should always kick the queue.) Yes, it can be removed from start(). Stefan
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); } /* @@ -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)
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(-)