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

[Pei Li]

Hi all,

My bad, I just submitted the kernel patch. If we are passing some generic command, still we have to add an additional field in the structure to indicate what is the unbatched version of this command, and the struct vhost_ioctls would be some command specific structure. In summary, the structure would be something like

struct vhost_cmd_batch {
    int ncmds;

    int cmd;
    struct vhost_ioctls[];
};

This is doable. Also, this is my first time submitting patches to open source, sorry about the mess in advance. That being said, feel free to throw questions / comments!

Thanks and best regards,

Pei

On Wed, Apr 19, 2023 at 9:19 PM Jason Wang <jasowang@redhat.com> wrote:

On Wed, Apr 19, 2023 at 11:33 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> 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.

I'd expect there's a kernel patch but I didn't see that?

If we want to go this way. Why simply have a more generic way, that is
introducing something like:

VHOST_CMD_BATCH which did something like

struct vhost_cmd_batch {
    int ncmds;
    struct vhost_ioctls[];
};

Then you can batch other ioctls other than GET_VRING_GROUP?

Thanks

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