| Message ID | 20221206081841.2458-2-longpeng2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | two optimizations to speed up the start time | expand |
On 6/12/22 09:18, Longpeng(Mike) via 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 time spend > on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs > and 3 vhost-vDPA generic devices[1] (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 | 40 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 7fb008bc9e..16f8391d86 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1507,7 +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 i, r, e; > + int i, n, r, e; > > /* We will pass the notifiers to the kernel, make sure that QEMU > * doesn't interfere. > @@ -1518,6 +1518,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); > @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > } > } > > + memory_region_transaction_commit(); > + > return 0; > fail_vq: > + /* save i for a second iteration after transaction is committed. */ > + n = i; > while (--i >= 0) { > e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, > false); > @@ -1536,8 +1546,18 @@ fail_vq: > error_report("vhost VQ %d notifier cleanup error: %d", i, -r); > } > assert (e >= 0); > - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i); > } > + > + /* > + * The transaction expects the ioeventfds to be open when it > + * commits. Do it now, before the cleanup loop. > + */ > + memory_region_transaction_commit(); > + > + while (--n >= 0) { > + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + n); > + } > + > virtio_device_release_ioeventfd(vdev); > fail: > return r; Similarly to patch #2, removing both goto statement in this function (as a preliminary patch) will 1/ simplify the code 2/ simplify reviewing your changes, resulting in something like: int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) { BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); int i, r, e; /* We will pass the notifiers to the kernel, make sure that QEMU * doesn't interfere. */ r = virtio_device_grab_ioeventfd(vdev); if (r < 0) { error_report("binding does not support host notifiers"); return r; } 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); if (r < 0) { error_report("vhost VQ %d notifier binding failed: %d", i, -r); while (--i >= 0) { 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); virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i); } virtio_device_release_ioeventfd(vdev); break; } } memory_region_transaction_commit(); return r; } What do you think? Regards, Phil.
在 2022/12/6 17:07, Philippe Mathieu-Daudé 写道: > On 6/12/22 09:18, Longpeng(Mike) via 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 time spend >> on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs >> and 3 vhost-vDPA generic devices[1] (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 | 40 ++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 38 insertions(+), 2 deletions(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 7fb008bc9e..16f8391d86 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1507,7 +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 i, r, e; >> + int i, n, r, e; >> /* We will pass the notifiers to the kernel, make sure that QEMU >> * doesn't interfere. >> @@ -1518,6 +1518,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); >> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev >> *hdev, VirtIODevice *vdev) >> } >> } >> + memory_region_transaction_commit(); >> + >> return 0; >> fail_vq: >> + /* save i for a second iteration after transaction is committed. */ >> + n = i; >> while (--i >= 0) { >> e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), >> hdev->vq_index + i, >> false); >> @@ -1536,8 +1546,18 @@ fail_vq: >> error_report("vhost VQ %d notifier cleanup error: %d", >> i, -r); >> } >> assert (e >= 0); >> - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >> hdev->vq_index + i); >> } >> + >> + /* >> + * The transaction expects the ioeventfds to be open when it >> + * commits. Do it now, before the cleanup loop. >> + */ >> + memory_region_transaction_commit(); >> + >> + while (--n >= 0) { >> + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >> hdev->vq_index + n); >> + } >> + >> virtio_device_release_ioeventfd(vdev); >> fail: >> return r; > > Similarly to patch #2, removing both goto statement in this function (as > a preliminary patch) will 1/ simplify the code 2/ simplify reviewing > your changes, resulting in something like: > > int vhost_dev_enable_notifiers(struct vhost_dev *hdev, > VirtIODevice *vdev) > { > BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); > int i, r, e; > > /* We will pass the notifiers to the kernel, make sure that QEMU > * doesn't interfere. > */ > r = virtio_device_grab_ioeventfd(vdev); > if (r < 0) { > error_report("binding does not support host notifiers"); > return r; > } > > 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); > if (r < 0) { > error_report("vhost VQ %d notifier binding failed: %d", > i, -r); > while (--i >= 0) { > 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); > virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), > hdev->vq_index + i); > } > virtio_device_release_ioeventfd(vdev); > break; > } > } > > memory_region_transaction_commit(); > > return r; > } > > What do you think? > Maybe we can use vhost_dev_disable_notifiers to further simplify the error path ? And we must commit before invoking virtio_bus_cleanup_host_notifier. > Regards, > > Phil. > .
On 6/12/22 11:28, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > > 在 2022/12/6 17:07, Philippe Mathieu-Daudé 写道: >> On 6/12/22 09:18, Longpeng(Mike) via 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 time spend >>> on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs >>> and 3 vhost-vDPA generic devices[1] (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 | 40 ++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 38 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index 7fb008bc9e..16f8391d86 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -1507,7 +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 i, r, e; >>> + int i, n, r, e; >>> /* We will pass the notifiers to the kernel, make sure that QEMU >>> * doesn't interfere. >>> @@ -1518,6 +1518,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); >>> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct >>> vhost_dev *hdev, VirtIODevice *vdev) >>> } >>> } >>> + memory_region_transaction_commit(); >>> + >>> return 0; >>> fail_vq: >>> + /* save i for a second iteration after transaction is committed. */ >>> + n = i; >>> while (--i >= 0) { >>> e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), >>> hdev->vq_index + i, >>> false); >>> @@ -1536,8 +1546,18 @@ fail_vq: >>> error_report("vhost VQ %d notifier cleanup error: %d", >>> i, -r); >>> } >>> assert (e >= 0); >>> - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >>> hdev->vq_index + i); >>> } >>> + >>> + /* >>> + * The transaction expects the ioeventfds to be open when it >>> + * commits. Do it now, before the cleanup loop. >>> + */ >>> + memory_region_transaction_commit(); >>> + >>> + while (--n >= 0) { >>> + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >>> hdev->vq_index + n); >>> + } >>> + >>> virtio_device_release_ioeventfd(vdev); >>> fail: >>> return r; >> >> Similarly to patch #2, removing both goto statement in this function >> (as a preliminary patch) will 1/ simplify the code 2/ simplify >> reviewing your changes, resulting in something like: >> >> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, >> VirtIODevice *vdev) >> { >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); >> int i, r, e; >> >> /* We will pass the notifiers to the kernel, make sure that QEMU >> * doesn't interfere. >> */ >> r = virtio_device_grab_ioeventfd(vdev); >> if (r < 0) { >> error_report("binding does not support host notifiers"); >> return r; >> } >> >> 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); >> if (r < 0) { >> error_report("vhost VQ %d notifier binding failed: %d", >> i, -r); >> while (--i >= 0) { >> 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); >> virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >> hdev->vq_index + i); >> } >> virtio_device_release_ioeventfd(vdev); >> break; >> } >> } >> >> memory_region_transaction_commit(); >> >> return r; >> } >> >> What do you think? >> > Maybe we can use vhost_dev_disable_notifiers to further simplify the > error path ? Good idea, but having the BusState resolved on each call seems a waste. Eventually factor it out and pass as argument ... > And we must commit before invoking virtio_bus_cleanup_host_notifier. ... but with that info on top, finally your original patch is simpler.
在 2022/12/6 18:45, Philippe Mathieu-Daudé 写道: > On 6/12/22 11:28, Longpeng (Mike, Cloud Infrastructure Service Product > Dept.) wrote: >> >> >> 在 2022/12/6 17:07, Philippe Mathieu-Daudé 写道: >>> On 6/12/22 09:18, Longpeng(Mike) via 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 time >>>> spend >>>> on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs >>>> and 3 vhost-vDPA generic devices[1] (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 | 40 ++++++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 38 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>>> index 7fb008bc9e..16f8391d86 100644 >>>> --- a/hw/virtio/vhost.c >>>> +++ b/hw/virtio/vhost.c >>>> @@ -1507,7 +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 i, r, e; >>>> + int i, n, r, e; >>>> /* We will pass the notifiers to the kernel, make sure that QEMU >>>> * doesn't interfere. >>>> @@ -1518,6 +1518,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); >>>> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct >>>> vhost_dev *hdev, VirtIODevice *vdev) >>>> } >>>> } >>>> + memory_region_transaction_commit(); >>>> + >>>> return 0; >>>> fail_vq: >>>> + /* save i for a second iteration after transaction is >>>> committed. */ >>>> + n = i; >>>> while (--i >= 0) { >>>> e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), >>>> hdev->vq_index + i, >>>> false); >>>> @@ -1536,8 +1546,18 @@ fail_vq: >>>> error_report("vhost VQ %d notifier cleanup error: %d", >>>> i, -r); >>>> } >>>> assert (e >= 0); >>>> - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >>>> hdev->vq_index + i); >>>> } >>>> + >>>> + /* >>>> + * The transaction expects the ioeventfds to be open when it >>>> + * commits. Do it now, before the cleanup loop. >>>> + */ >>>> + memory_region_transaction_commit(); >>>> + >>>> + while (--n >= 0) { >>>> + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >>>> hdev->vq_index + n); >>>> + } >>>> + >>>> virtio_device_release_ioeventfd(vdev); >>>> fail: >>>> return r; >>> >>> Similarly to patch #2, removing both goto statement in this function >>> (as a preliminary patch) will 1/ simplify the code 2/ simplify >>> reviewing your changes, resulting in something like: >>> >>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, >>> VirtIODevice *vdev) >>> { >>> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); >>> int i, r, e; >>> >>> /* We will pass the notifiers to the kernel, make sure that QEMU >>> * doesn't interfere. >>> */ >>> r = virtio_device_grab_ioeventfd(vdev); >>> if (r < 0) { >>> error_report("binding does not support host notifiers"); >>> return r; >>> } >>> >>> 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); >>> if (r < 0) { >>> error_report("vhost VQ %d notifier binding failed: %d", >>> i, -r); >>> while (--i >= 0) { >>> 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); >>> virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), >>> hdev->vq_index + i); >>> } >>> virtio_device_release_ioeventfd(vdev); >>> break; >>> } >>> } >>> >>> memory_region_transaction_commit(); >>> >>> return r; >>> } >>> >>> What do you think? >>> >> Maybe we can use vhost_dev_disable_notifiers to further simplify the >> error path ? > > Good idea, but having the BusState resolved on each call seems a waste. > Eventually factor it out and pass as argument ... > >> And we must commit before invoking virtio_bus_cleanup_host_notifier. > > ... but with that info on top, finally your original patch is simpler. Yes, I'll try in next version, thanks. > .
On Wed, Dec 07, 2022 at 08:22:18AM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > > And we must commit before invoking virtio_bus_cleanup_host_notifier. > > > > ... but with that info on top, finally your original patch is simpler. > > Yes, I'll try in next version, thanks. OK so I'm waiting for v3.
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 7fb008bc9e..16f8391d86 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1507,7 +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 i, r, e; + int i, n, r, e; /* We will pass the notifiers to the kernel, make sure that QEMU * doesn't interfere. @@ -1518,6 +1518,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); @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) } } + memory_region_transaction_commit(); + return 0; fail_vq: + /* save i for a second iteration after transaction is committed. */ + n = i; while (--i >= 0) { e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i, false); @@ -1536,8 +1546,18 @@ fail_vq: error_report("vhost VQ %d notifier cleanup error: %d", i, -r); } assert (e >= 0); - virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i); } + + /* + * The transaction expects the ioeventfds to be open when it + * commits. Do it now, before the cleanup loop. + */ + memory_region_transaction_commit(); + + while (--n >= 0) { + virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + n); + } + virtio_device_release_ioeventfd(vdev); fail: return r; @@ -1553,6 +1573,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 +1586,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); }