On Thu, Apr 20, 2023 at 1:25 PM Pei Li <pe...@andrew.cmu.edu> wrote:
>
> Hi all,
>
> My bad, I just submitted the kernel patch.
Please cc maintainers. You can get it via scripts/get_maintainer.pl
> 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,
Right.
> 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.
Let's try that?
> 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!
You can get more instructions on how to contribute patches through
Documentation/process/submitting-patches.rst.
Happy hacking!
Thanks
>
> Thanks and best regards,
> Pei
>
> On Wed, Apr 19, 2023 at 9:19 PM Jason Wang <jasow...@redhat.com> wrote:
>>
>> 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/
>> >
>>
>>
