hw/virtio/vhost.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
From: Longpeng <longpeng2@huawei.com>
This allows the vhost device to batch the setup of all its host notifiers.
This significantly reduces the device starting time, e.g. the vhost-vDPA
generic device [1] start time reduce from 376ms to 9.1ms for a VM with
64 vCPUs and 3 vDPA device(64vq per device).
[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
Signed-off-by: Longpeng <longpeng2@huawei.com>
---
hw/virtio/vhost.c | 39 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d1c4c20b8c..bf82d9b176 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1507,6 +1507,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
{
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+ int vq_init_count = 0;
int i, r, e;
/* We will pass the notifiers to the kernel, make sure that QEMU
@@ -1518,6 +1519,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
goto fail;
}
+ /*
+ * 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,
true);
@@ -1525,19 +1532,33 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
error_report("vhost VQ %d notifier binding failed: %d", i, -r);
goto fail_vq;
}
+
+ vq_init_count++;
}
+ memory_region_transaction_commit();
+
return 0;
fail_vq:
- while (--i >= 0) {
+ for (i = 0; i < vq_init_count; i++) {
e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
false);
if (e < 0) {
error_report("vhost VQ %d notifier cleanup error: %d", i, -r);
}
assert (e >= 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 < vq_init_count; i++) {
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
}
+
virtio_device_release_ioeventfd(vdev);
fail:
return r;
@@ -1553,6 +1574,12 @@ 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);
@@ -1560,8 +1587,18 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
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);
}
--
2.23.0
On Fri, Nov 18, 2022 at 10:49 PM Longpeng(Mike) <longpeng2@huawei.com> wrote: > > From: Longpeng <longpeng2@huawei.com> > > This allows the vhost device to batch the setup of all its host notifiers. > This significantly reduces the device starting time, e.g. the vhost-vDPA > generic device [1] start time reduce from 376ms to 9.1ms for a VM with > 64 vCPUs and 3 vDPA device(64vq per device). Great, I think we need to do this for host_notifiers_mr as well. This helps for the case when the notification area could be mapped directly to guests. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index d1c4c20b8c..bf82d9b176 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1507,6 +1507,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev) > int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > { > BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); > + int vq_init_count = 0; > int i, r, e; > > /* We will pass the notifiers to the kernel, make sure that QEMU > @@ -1518,6 +1519,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > goto fail; > } > > + /* > + * 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, > true); > @@ -1525,19 +1532,33 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > error_report("vhost VQ %d notifier binding failed: %d", i, -r); > goto fail_vq; > } > + > + vq_init_count++; > } Nit, the name needs some tweak, it's actually the number of the host notifiers that is initialized. And we can count it out of the loop. > > + memory_region_transaction_commit(); > + > return 0; > fail_vq: > - while (--i >= 0) { > + for (i = 0; i < vq_init_count; i++) { It looks to me there's no need for this change. Others look good. Thanks > e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, > false); > if (e < 0) { > error_report("vhost VQ %d notifier cleanup error: %d", i, -r); > } > assert (e >= 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 < vq_init_count; i++) { > virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i); > } > + > virtio_device_release_ioeventfd(vdev); > fail: > return r; > @@ -1553,6 +1574,12 @@ 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); > @@ -1560,8 +1587,18 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > 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); > } > > -- > 2.23.0 >
在 2022/11/21 12:01, Jason Wang 写道: > On Fri, Nov 18, 2022 at 10:49 PM Longpeng(Mike) <longpeng2@huawei.com> wrote: >> >> From: Longpeng <longpeng2@huawei.com> >> >> This allows the vhost device to batch the setup of all its host notifiers. >> This significantly reduces the device starting time, e.g. the vhost-vDPA >> generic device [1] start time reduce from 376ms to 9.1ms for a VM with >> 64 vCPUs and 3 vDPA device(64vq per device). > > Great, I think we need to do this for host_notifiers_mr as well. This > helps for the case when the notification area could be mapped directly > to guests. > Batch and commit once for host_notifiers_mrs can reduce the cost from 423ms to 32ms, testing on vdpasim_blk (3 devices and 64 vqs per device) with doorbell passthrough support. I'll append a patch in the next version. >> >> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html >> >> Signed-off-by: Longpeng <longpeng2@huawei.com> >> --- >> hw/virtio/vhost.c | 39 ++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 38 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index d1c4c20b8c..bf82d9b176 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1507,6 +1507,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev) >> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >> { >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); >> + int vq_init_count = 0; >> int i, r, e; >> >> /* We will pass the notifiers to the kernel, make sure that QEMU >> @@ -1518,6 +1519,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >> goto fail; >> } >> >> + /* >> + * 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, >> true); >> @@ -1525,19 +1532,33 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >> error_report("vhost VQ %d notifier binding failed: %d", i, -r); >> goto fail_vq; >> } >> + >> + vq_init_count++; >> } > > Nit, the name needs some tweak, it's actually the number of the host > notifiers that is initialized. And we can count it out of the loop. > Ok, I will refer to virtio_device_start_ioeventfd_impl(). >> >> + memory_region_transaction_commit(); >> + >> return 0; >> fail_vq: >> - while (--i >= 0) { >> + for (i = 0; i < vq_init_count; i++) { > > It looks to me there's no need for this change. > > Others look good. > > Thanks > >> e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, >> false); >> if (e < 0) { >> error_report("vhost VQ %d notifier cleanup error: %d", i, -r); >> } >> assert (e >= 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 < vq_init_count; i++) { >> virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i); >> } >> + >> virtio_device_release_ioeventfd(vdev); >> fail: >> return r; >> @@ -1553,6 +1574,12 @@ 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); >> @@ -1560,8 +1587,18 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >> 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); >> } >> >> -- >> 2.23.0 >> > > .
On Mon, Nov 28, 2022 at 5:08 PM Longpeng (Mike, Cloud Infrastructure Service Product Dept.) <longpeng2@huawei.com> wrote: > > > > 在 2022/11/21 12:01, Jason Wang 写道: > > On Fri, Nov 18, 2022 at 10:49 PM Longpeng(Mike) <longpeng2@huawei.com> wrote: > >> > >> From: Longpeng <longpeng2@huawei.com> > >> > >> This allows the vhost device to batch the setup of all its host notifiers. > >> This significantly reduces the device starting time, e.g. the vhost-vDPA > >> generic device [1] start time reduce from 376ms to 9.1ms for a VM with > >> 64 vCPUs and 3 vDPA device(64vq per device). > > > > Great, I think we need to do this for host_notifiers_mr as well. This > > helps for the case when the notification area could be mapped directly > > to guests. > > > Batch and commit once for host_notifiers_mrs can reduce the cost from > 423ms to 32ms, testing on vdpasim_blk (3 devices and 64 vqs per device) > with doorbell passthrough support. > I'll append a patch in the next version. Great. Thanks > > >> > >> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html > >> > >> Signed-off-by: Longpeng <longpeng2@huawei.com> > >> --- > >> hw/virtio/vhost.c | 39 ++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 38 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >> index d1c4c20b8c..bf82d9b176 100644 > >> --- a/hw/virtio/vhost.c > >> +++ b/hw/virtio/vhost.c > >> @@ -1507,6 +1507,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev) > >> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > >> { > >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); > >> + int vq_init_count = 0; > >> int i, r, e; > >> > >> /* We will pass the notifiers to the kernel, make sure that QEMU > >> @@ -1518,6 +1519,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > >> goto fail; > >> } > >> > >> + /* > >> + * 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, > >> true); > >> @@ -1525,19 +1532,33 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > >> error_report("vhost VQ %d notifier binding failed: %d", i, -r); > >> goto fail_vq; > >> } > >> + > >> + vq_init_count++; > >> } > > > > Nit, the name needs some tweak, it's actually the number of the host > > notifiers that is initialized. And we can count it out of the loop. > > > Ok, I will refer to virtio_device_start_ioeventfd_impl(). > > >> > >> + memory_region_transaction_commit(); > >> + > >> return 0; > >> fail_vq: > >> - while (--i >= 0) { > >> + for (i = 0; i < vq_init_count; i++) { > > > > It looks to me there's no need for this change. > > > > Others look good. > > > > Thanks > > > >> e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, > >> false); > >> if (e < 0) { > >> error_report("vhost VQ %d notifier cleanup error: %d", i, -r); > >> } > >> assert (e >= 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 < vq_init_count; i++) { > >> virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i); > >> } > >> + > >> virtio_device_release_ioeventfd(vdev); > >> fail: > >> return r; > >> @@ -1553,6 +1574,12 @@ 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); > >> @@ -1560,8 +1587,18 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > >> 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); > >> } > >> > >> -- > >> 2.23.0 > >> > > > > . >
On Fri, 18 Nov 2022 at 09:51, Longpeng(Mike) via <qemu-devel@nongnu.org> wrote: > > From: Longpeng <longpeng2@huawei.com> > > This allows the vhost device to batch the setup of all its host notifiers. > This significantly reduces the device starting time, e.g. the vhost-vDPA > generic device [1] start time reduce from 376ms to 9.1ms for a VM with > 64 vCPUs and 3 vDPA device(64vq per device). > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
© 2016 - 2024 Red Hat, Inc.