Re: [PATCH 1/2] Reduce vdpa initialization / startup overhead

[Eugenio Perez Martin]

On Wed, Apr 19, 2023 at 12:56 AM <peili.dev@gmail.com> wrote:
>
> From: Pei Li <peili.dev@gmail.com>
>
> Currently, part of the vdpa initialization / startup process
> needs to trigger many ioctls per vq, which is very inefficient
> and causing unnecessary context switch between user mode and
> kernel mode.
>
> This patch creates an additional ioctl() command, namely
> VHOST_VDPA_GET_VRING_GROUP_BATCH, that will batching
> commands of VHOST_VDPA_GET_VRING_GROUP into a single
> ioctl() call.
>

It seems to me you forgot to send the 0/2 cover letter :).

Please include the time we save thanks to avoiding the repetitive
ioctls in each patch.

CCing Jason and Michael.

> Signed-off-by: Pei Li <peili.dev@gmail.com>
> ---
>  hw/virtio/vhost-vdpa.c                       | 31 +++++++++++++++-----
>  include/standard-headers/linux/vhost_types.h |  3 ++
>  linux-headers/linux/vhost.h                  |  7 +++++
>  3 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index bc6bad23d5..6d45ff8539 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -679,7 +679,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev 
> *dev)
>      uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
>          0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
>          0x1ULL << VHOST_BACKEND_F_IOTLB_ASID |
> -        0x1ULL << VHOST_BACKEND_F_SUSPEND;
> +        0x1ULL << VHOST_BACKEND_F_SUSPEND |
> +        0x1ULL << VHOST_BACKEND_F_IOCTL_BATCH;
>      int r;
>
>      if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
> @@ -731,14 +732,28 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev 
> *dev, int idx)
>
>  static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>  {
> -    int i;
> +    int i, nvqs = dev->nvqs;
> +    uint64_t backend_features = dev->backend_cap;
> +
>      trace_vhost_vdpa_set_vring_ready(dev);
> -    for (i = 0; i < dev->nvqs; ++i) {
> -        struct vhost_vring_state state = {
> -            .index = dev->vq_index + i,
> -            .num = 1,
> -        };
> -        vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> +
> +    if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOCTL_BATCH))) {
> +        for (i = 0; i < nvqs; ++i) {
> +            struct vhost_vring_state state = {
> +                .index = dev->vq_index + i,
> +                .num = 1,
> +            };
> +            vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> +        }
> +    } else {
> +        struct vhost_vring_state states[nvqs + 1];
> +        states[0].num = nvqs;
> +        for (i = 1; i <= nvqs; ++i) {
> +            states[i].index = dev->vq_index + i - 1;
> +            states[i].num = 1;
> +        }
> +

I think it's more clear to share the array of vhost_vring_state
states[nvqs + 1], and then fill it either in one shot with
VHOST_VDPA_SET_VRING_ENABLE_BATCH or individually with
VHOST_VDPA_SET_VRING_ENABLE.

The same feedback goes for VHOST_VDPA_GET_VRING_GROUP_BATCH in the next patch.

> +        vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE_BATCH, &states[0]);
>      }
>      return 0;

This comment is previous to your patch but I just realized we don't
check the return value of vhost_vdpa_call. Maybe it is worth
considering addressing that in this series too.

>  }
> diff --git a/include/standard-headers/linux/vhost_types.h 
> b/include/standard-headers/linux/vhost_types.h
> index c41a73fe36..068d0e1ceb 100644
> --- a/include/standard-headers/linux/vhost_types.h
> +++ b/include/standard-headers/linux/vhost_types.h
> @@ -164,4 +164,7 @@ struct vhost_vdpa_iova_range {
>  /* Device can be suspended */
>  #define VHOST_BACKEND_F_SUSPEND  0x4
>
> +/* IOCTL requests can be batched */
> +#define VHOST_BACKEND_F_IOCTL_BATCH 0x6
> +
>  #endif
> diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
> index f9f115a7c7..4c9ddd0a0e 100644
> --- a/linux-headers/linux/vhost.h
> +++ b/linux-headers/linux/vhost.h
> @@ -180,4 +180,11 @@
>   */
>  #define VHOST_VDPA_SUSPEND             _IO(VHOST_VIRTIO, 0x7D)
>
> +/* Batch version of VHOST_VDPA_SET_VRING_ENABLE
> + *
> + * Enable/disable the ring while batching the commands.
> + */
> +#define VHOST_VDPA_SET_VRING_ENABLE_BATCH      _IOW(VHOST_VIRTIO, 0x7F, \
> +                                            struct vhost_vring_state)
> +

These headers should be updated with update-linux-headers.sh. To be
done that way we need the changes merged in the kernel before.

We can signal that the series is not ready for inclusion with the
subject [RFC. PATCH], like [1], and note it in the commit message.
That way, you can keep updating the header manually for demonstration
purposes.

Thanks!

[1] 
https://lore.kernel.org/qemu-devel/20220413163206.1958254-23-eperezma@redhat.com/