| Message ID | 20221227072015.3134-3-longpeng2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | two optimizations to speed up the start time | expand |
On 27/12/22 08:20, Longpeng(Mike) 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 (vdpa_sim_blk, 64vq per device) > > Signed-off-by: Longpeng <longpeng2@huawei.com> > --- > hw/virtio/vhost.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 5994559da8..064d4abe5c 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > return 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, > true); > if (r < 0) { > error_report("vhost VQ %d notifier binding failed: %d", i, -r); > + memory_region_transaction_commit(); > vhost_dev_disable_notifiers(hdev, vdev); Could we 'break' here, ... > return r; > } > } > > + memory_region_transaction_commit(); > + > return 0; ... and return 'r' here? > } Otherwise LGTM.
On Tue, Dec 27, 2022 at 05:43:57PM +0100, Philippe Mathieu-Daudé wrote: > On 27/12/22 08:20, Longpeng(Mike) 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 (vdpa_sim_blk, 64vq per device) > > > > Signed-off-by: Longpeng <longpeng2@huawei.com> > > --- > > hw/virtio/vhost.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 5994559da8..064d4abe5c 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) > > return 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, > > true); > > if (r < 0) { > > error_report("vhost VQ %d notifier binding failed: %d", i, -r); > > + memory_region_transaction_commit(); > > vhost_dev_disable_notifiers(hdev, vdev); > > Could we 'break' here, ... > > > return r; > > } > > } > > + memory_region_transaction_commit(); > > + > > return 0; > > ... and return 'r' here? won't this commit twice? seems ugly ... > > } > > Otherwise LGTM.
On 27/12/22 18:54, Michael S. Tsirkin wrote: > On Tue, Dec 27, 2022 at 05:43:57PM +0100, Philippe Mathieu-Daudé wrote: >> On 27/12/22 08:20, Longpeng(Mike) 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 (vdpa_sim_blk, 64vq per device) >>> >>> Signed-off-by: Longpeng <longpeng2@huawei.com> >>> --- >>> hw/virtio/vhost.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index 5994559da8..064d4abe5c 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) >>> return 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, >>> true); >>> if (r < 0) { >>> error_report("vhost VQ %d notifier binding failed: %d", i, -r); >>> + memory_region_transaction_commit(); >>> vhost_dev_disable_notifiers(hdev, vdev); >> >> Could we 'break' here, ... >> >>> return r; >>> } >>> } >>> + memory_region_transaction_commit(); >>> + >>> return 0; >> >> ... and return 'r' here? > > > won't this commit twice? seems ugly ... Twice? I meant keep the begin/commit() around the for() to have only *one* commit() call instead of 2: -- >8 -- + 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); vhost_dev_disable_notifiers(hdev, vdev); - return r; + break; } } + memory_region_transaction_commit(); + - return 0; + return r; } --- Anyhow, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
在 2022/12/28 21:12, Philippe Mathieu-Daudé 写道: > On 27/12/22 18:54, Michael S. Tsirkin wrote: >> On Tue, Dec 27, 2022 at 05:43:57PM +0100, Philippe Mathieu-Daudé wrote: >>> On 27/12/22 08:20, Longpeng(Mike) 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 (vdpa_sim_blk, 64vq per device) >>>> >>>> Signed-off-by: Longpeng <longpeng2@huawei.com> >>>> --- >>>> hw/virtio/vhost.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>>> index 5994559da8..064d4abe5c 100644 >>>> --- a/hw/virtio/vhost.c >>>> +++ b/hw/virtio/vhost.c >>>> @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct >>>> vhost_dev *hdev, VirtIODevice *vdev) >>>> return 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, >>>> true); >>>> if (r < 0) { >>>> error_report("vhost VQ %d notifier binding failed: >>>> %d", i, -r); >>>> + memory_region_transaction_commit(); >>>> vhost_dev_disable_notifiers(hdev, vdev); >>> >>> Could we 'break' here, ... >>> >>>> return r; >>>> } >>>> } >>>> + memory_region_transaction_commit(); >>>> + >>>> return 0; >>> >>> ... and return 'r' here? >> >> >> won't this commit twice? seems ugly ... > > Twice? I meant keep the begin/commit() around the for() to have > only *one* commit() call instead of 2: > There's a transaction in vhost_dev_disable_notifiers() too, We must commit the outter transaction before invoking it, you can see the comment in it. > -- >8 -- > + 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); > vhost_dev_disable_notifiers(hdev, vdev); > - return r; > + break; > } > } > > + memory_region_transaction_commit(); > + > - return 0; > + return r; > } > --- > > Anyhow, > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > .
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 5994559da8..064d4abe5c 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev) return 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, true); if (r < 0) { error_report("vhost VQ %d notifier binding failed: %d", i, -r); + memory_region_transaction_commit(); vhost_dev_disable_notifiers(hdev, vdev); return r; } } + memory_region_transaction_commit(); + return 0; } @@ -1585,6 +1594,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); @@ -1592,6 +1607,15 @@ 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);