On Wed, Apr 19, 2023 at 11:33 PM Eugenio Perez Martin
<epere...@redhat.com> wrote:
>
> On Wed, Apr 19, 2023 at 12:56 AM <peili....@gmail.com> wrote:
> >
> > From: Pei Li <peili....@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....@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-epere...@redhat.com/
>
