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.0On 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.