Re: [PATCH v2 1/2] vhost: configure all host notifiers in a single MR transaction

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.