Re: [PATCH] vhost: configure all host notifiers in a single MR transaction

[Jason Wang]

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