hw/virtio/vhost.c | 65 ++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 29 deletions(-)
in vhost_dev_enable_notifiers(), if virtio_bus_set_host_notifier(true)
fails, we call vhost_dev_disable_notifiers() that executes
virtio_bus_set_host_notifier(false) on all queues, even on queues that
have failed to be initialized.
This triggers a core dump in memory_region_del_eventfd():
virtio_bus_set_host_notifier: unable to init event notifier: Too many open files (-24)
vhost VQ 1 notifier binding failed: 24
.../softmmu/memory.c:2611: memory_region_del_eventfd: Assertion `i != mr->ioeventfd_nb' failed.
Fix the problem by providing to vhost_dev_disable_notifiers() the
number of queues to disable.
Fixes: 8771589b6f81 ("vhost: simplify vhost_dev_enable_notifiers")
Cc: longpeng2@huawei.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
hw/virtio/vhost.c | 65 ++++++++++++++++++++++++++---------------------
1 file changed, 36 insertions(+), 29 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 746d130c7406..02ac68a21f54 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1531,6 +1531,40 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
memset(hdev, 0, sizeof(struct vhost_dev));
}
+static void vhost_dev_disable_notifiers_nvqs(struct vhost_dev *hdev,
+ VirtIODevice *vdev,
+ unsigned int nvqs)
+{
+ BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+ int i, r;
+
+ /*
+ * Batch all the host notifiers in a single transaction to avoid
+ * quadratic time complexity in address_space_update_ioeventfds().
+ */
+ memory_region_transaction_begin();
+
+ for (i = 0; i < nvqs; ++i) {
+ r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
+ false);
+ if (r < 0) {
+ error_report("vhost VQ %d notifier cleanup failed: %d", i, -r);
+ }
+ assert(r >= 0);
+ }
+
+ /*
+ * The transaction expects the ioeventfds to be open when it
+ * commits. Do it now, before the cleanup loop.
+ */
+ memory_region_transaction_commit();
+
+ for (i = 0; i < nvqs; ++i) {
+ virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
+ }
+ virtio_device_release_ioeventfd(vdev);
+}
+
/* Stop processing guest IO notifications in qemu.
* Start processing them in vhost in kernel.
*/
@@ -1560,7 +1594,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
if (r < 0) {
error_report("vhost VQ %d notifier binding failed: %d", i, -r);
memory_region_transaction_commit();
- vhost_dev_disable_notifiers(hdev, vdev);
+ vhost_dev_disable_notifiers_nvqs(hdev, vdev, i);
return r;
}
}
@@ -1577,34 +1611,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
*/
void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
{
- BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
- int i, r;
-
- /*
- * Batch all the host notifiers in a single transaction to avoid
- * quadratic time complexity in address_space_update_ioeventfds().
- */
- memory_region_transaction_begin();
-
- for (i = 0; i < hdev->nvqs; ++i) {
- r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
- false);
- if (r < 0) {
- error_report("vhost VQ %d notifier cleanup failed: %d", i, -r);
- }
- assert (r >= 0);
- }
-
- /*
- * The transaction expects the ioeventfds to be open when it
- * commits. Do it now, before the cleanup loop.
- */
- memory_region_transaction_commit();
-
- for (i = 0; i < hdev->nvqs; ++i) {
- virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
- }
- virtio_device_release_ioeventfd(vdev);
+ vhost_dev_disable_notifiers_nvqs(hdev, vdev, hdev->nvqs);
}
/* Test and clear event pending status.
--
2.39.2
02.06.2023 19:27, Laurent Vivier wrote: > in vhost_dev_enable_notifiers(), if virtio_bus_set_host_notifier(true) > fails, we call vhost_dev_disable_notifiers() that executes > virtio_bus_set_host_notifier(false) on all queues, even on queues that > have failed to be initialized. > > This triggers a core dump in memory_region_del_eventfd(): > > virtio_bus_set_host_notifier: unable to init event notifier: Too many open files (-24) > vhost VQ 1 notifier binding failed: 24 > .../softmmu/memory.c:2611: memory_region_del_eventfd: Assertion `i != mr->ioeventfd_nb' failed. > > Fix the problem by providing to vhost_dev_disable_notifiers() the > number of queues to disable. > > Fixes: 8771589b6f81 ("vhost: simplify vhost_dev_enable_notifiers") > Cc: longpeng2@huawei.com > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/virtio/vhost.c | 65 ++++++++++++++++++++++++++--------------------- > 1 file changed, 36 insertions(+), 29 deletions(-) Is this one a candidate for -stable? The diffstat is somewhat large but it is just moving bit of code around. Thanks, /mjt
On Wed, Jun 07, 2023 at 12:32:31PM +0300, Michael Tokarev wrote: > 02.06.2023 19:27, Laurent Vivier wrote: > > in vhost_dev_enable_notifiers(), if virtio_bus_set_host_notifier(true) > > fails, we call vhost_dev_disable_notifiers() that executes > > virtio_bus_set_host_notifier(false) on all queues, even on queues that > > have failed to be initialized. > > > > This triggers a core dump in memory_region_del_eventfd(): > > > > virtio_bus_set_host_notifier: unable to init event notifier: Too many open files (-24) > > vhost VQ 1 notifier binding failed: 24 > > .../softmmu/memory.c:2611: memory_region_del_eventfd: Assertion `i != mr->ioeventfd_nb' failed. > > > > Fix the problem by providing to vhost_dev_disable_notifiers() the > > number of queues to disable. > > > > Fixes: 8771589b6f81 ("vhost: simplify vhost_dev_enable_notifiers") > > Cc: longpeng2@huawei.com > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > --- > > hw/virtio/vhost.c | 65 ++++++++++++++++++++++++++--------------------- > > 1 file changed, 36 insertions(+), 29 deletions(-) > > Is this one a candidate for -stable? > > The diffstat is somewhat large but it is just moving bit of code around. I'd say so, yes. > Thanks, > > /mjt
On 2/6/23 18:27, Laurent Vivier wrote: > in vhost_dev_enable_notifiers(), if virtio_bus_set_host_notifier(true) > fails, we call vhost_dev_disable_notifiers() that executes > virtio_bus_set_host_notifier(false) on all queues, even on queues that > have failed to be initialized. > > This triggers a core dump in memory_region_del_eventfd(): > > virtio_bus_set_host_notifier: unable to init event notifier: Too many open files (-24) > vhost VQ 1 notifier binding failed: 24 > .../softmmu/memory.c:2611: memory_region_del_eventfd: Assertion `i != mr->ioeventfd_nb' failed. > > Fix the problem by providing to vhost_dev_disable_notifiers() the > number of queues to disable. > > Fixes: 8771589b6f81 ("vhost: simplify vhost_dev_enable_notifiers") > Cc: longpeng2@huawei.com > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/virtio/vhost.c | 65 ++++++++++++++++++++++++++--------------------- > 1 file changed, 36 insertions(+), 29 deletions(-) I'd rather have 2 patches, one factoring the new helper out and the 2nd fixing the bug. If you ever need to respin... Anyhow, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2024 Red Hat, Inc.