Re: [PATCH v2 2/2] vdpa: send CVQ state load commands in parallel

[Hawkins Jiawei]

On 2023/5/18 14:12, Jason Wang wrote:
> On Thu, May 18, 2023 at 2:00 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
>>
>> On Thu, May 18, 2023 at 7:47 AM Jason Wang <jasowang@redhat.com> wrote:
>>>
>>> On Wed, May 17, 2023 at 11:02 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>>>
>>>> Sorry for forgetting cc when replying to the email.
>>>>
>>>> On Wed, 17 May 2023 at 16:22, Eugenio Perez Martin <eperezma@redhat.com> 
>>>> wrote:
>>>>>
>>>>> On Wed, May 17, 2023 at 7:22 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>
>>>>>> On Sat, May 6, 2023 at 10:07 PM Hawkins Jiawei <yin31149@gmail.com> 
>>>>>> wrote:
>>>>>>>
>>>>>>> This patch introduces the vhost_vdpa_net_cvq_add() and
>>>>>>> refactors the vhost_vdpa_net_load*(), so that QEMU can
>>>>>>> send CVQ state load commands in parallel.
>>>>>>>
>>>>>>> To be more specific, this patch introduces vhost_vdpa_net_cvq_add()
>>>>>>> to add SVQ control commands to SVQ and kick the device,
>>>>>>> but does not poll the device used buffers. QEMU will not
>>>>>>> poll and check the device used buffers in vhost_vdpa_net_load()
>>>>>>> until all CVQ state load commands have been sent to the device.
>>>>>>>
>>>>>>> What's more, in order to avoid buffer overwriting caused by
>>>>>>> using `svq->cvq_cmd_out_buffer` and `svq->status` as the
>>>>>>> buffer for all CVQ state load commands when sending
>>>>>>> CVQ state load commands in parallel, this patch introduces
>>>>>>> `out_cursor` and `in_cursor` in vhost_vdpa_net_load(),
>>>>>>> pointing to the available buffer for in descriptor and
>>>>>>> out descriptor, so that different CVQ state load commands can
>>>>>>> use their unique buffer.
>>>>>>>
>>>>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1578
>>>>>>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>>>>>>> ---
>>>>>>>   net/vhost-vdpa.c | 152 +++++++++++++++++++++++++++++++++++++----------
>>>>>>>   1 file changed, 120 insertions(+), 32 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>>>>> index 10804c7200..14e31ca5c5 100644
>>>>>>> --- a/net/vhost-vdpa.c
>>>>>>> +++ b/net/vhost-vdpa.c
>>>>>>> @@ -590,6 +590,44 @@ static void vhost_vdpa_net_cvq_stop(NetClientState 
>>>>>>> *nc)
>>>>>>>       vhost_vdpa_net_client_stop(nc);
>>>>>>>   }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * vhost_vdpa_net_cvq_add() adds SVQ control commands to SVQ,
>>>>>>> + * kicks the device but does not poll the device used buffers.
>>>>>>> + *
>>>>>>> + * Return the number of elements added to SVQ if success.
>>>>>>> + */
>>>>>>> +static int vhost_vdpa_net_cvq_add(VhostVDPAState *s,
>>>>>>> +                                void **out_cursor, size_t out_len,
>>>>>>
>>>>>> Can we track things like cursors in e.g VhostVDPAState ?
>>>>>>
>>>>>
>>>>> Cursors will only be used at device startup. After that, cursors are
>>>>> always the full buffer. Do we want to "pollute" VhostVDPAState with
>>>>> things that will not be used after the startup?
>>>
>>> So it's the cursor of the cvq_cmd_out_buffer, it would be more elegant
>>> to keep it with where the cvq_cmd_out_buffer is placed. It can avoid
>>> passing cursors in several levels.
>>> >>
>> That's right, there is no reason to update at vhost_vdpa_net_cvq_add.
>> It can be done at the caller.

But in any case, these cursors need to be passed as alone parameters to
vhost_vdpa_net_cvq_add(), so that they can be accessed for
`struct iovec` `iov_base` field, right? Considering that
we always pass these parameters, so I also update them together in
vhost_vdpa_net_cvq_add() in this patch.

If we want to avoid passing cursors in several levels, is it okay
to backup `cvq_cmd_out_buffer` and `status` in vhost_vdpa_net_load(),
access and update them through `VhostVDPAState` directly during loading,
and restore them when finishing loading.

>>
>>> Or it would be even better to have some buffer allocation helpers to
>>> alloc and free in and out buffers.
>>>
>>
>> I think that's overkill, as the buffers are always the in and out CVQ
>> buffers. They are allocated at qemu initialization, and mapped /
>> unmapped at device start / stop for now.
>
> It's not a must, we can do it if it has more users for sure.
>
>>
>> To manage them dynamically would need code to map them at any time etc.
>
> Thanks
>
>>
>> Thanks!
>>
>>> Thanks
>>>
>>>>>
>>>>> Maybe we can create a struct for them and then pass just this struct?
>>>>
>>>> Yes, Currently, the new version of vhost_vdpa_net_cvq_add() is only
>>>> called in vhost_vdpa_net_load() at startup, so these cursors will not be
>>>> used after startup.
>>>>
>>>> If needed, we can create a `VhostVDPACursor` as suggested by Eugenio.
>>>>
>>>>>
>>>>> Thanks!
>>>>>
>>>>>>> +                                virtio_net_ctrl_ack **in_cursor, 
>>>>>>> size_t in_len)
>>>>>>> +{
>>>>>>> +    /* Buffers for the device */
>>>>>>> +    const struct iovec out = {
>>>>>>> +        .iov_base = *out_cursor,
>>>>>>> +        .iov_len = out_len,
>>>>>>> +    };
>>>>>>> +    const struct iovec in = {
>>>>>>> +        .iov_base = *in_cursor,
>>>>>>> +        .iov_len = sizeof(virtio_net_ctrl_ack),
>>>>>>> +    };
>>>>>>> +    VhostShadowVirtqueue *svq = 
>>>>>>> g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0);
>>>>>>> +    int r;
>>>>>>> +
>>>>>>> +    r = vhost_svq_add(svq, &out, 1, &in, 1, NULL);
>>>>>>> +    if (unlikely(r != 0)) {
>>>>>>> +        if (unlikely(r == -ENOSPC)) {
>>>>>>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device 
>>>>>>> queue\n",
>>>>>>> +                          __func__);
>>>>>>> +        }
>>>>>>> +        return r;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* Update the cursor */
>>>>>>> +    *out_cursor += out_len;
>>>>>>> +    *in_cursor += 1;
>>>>>>> +
>>>>>>> +    return 1;
>>>>>>> +}
>>>>>>> +
>>>>>>>   /**
>>>>>>>    * vhost_vdpa_net_cvq_add_and_wait() adds SVQ control commands to SVQ,
>>>>>>>    * kicks the device and polls the device used buffers.
>>>>>>> @@ -628,69 +666,82 @@ static ssize_t 
>>>>>>> vhost_vdpa_net_cvq_add_and_wait(VhostVDPAState *s,
>>>>>>>       return vhost_svq_poll(svq);
>>>>>>>   }
>>>>>>>
>>>>>>> -static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t 
>>>>>>> class,
>>>>>>> -                                       uint8_t cmd, const void *data,
>>>>>>> -                                       size_t data_size)
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * vhost_vdpa_net_load_cmd() restores the NIC state through SVQ.
>>>>>>> + *
>>>>>>> + * Return the number of elements added to SVQ if success.
>>>>>>> + */
>>>>>>> +static int vhost_vdpa_net_load_cmd(VhostVDPAState *s,
>>>>>>> +                                void **out_cursor, uint8_t class, 
>>>>>>> uint8_t cmd,
>>>>>>> +                                const void *data, size_t data_size,
>>>>>>> +                                virtio_net_ctrl_ack **in_cursor)
>>>>>>>   {
>>>>>>>       const struct virtio_net_ctrl_hdr ctrl = {
>>>>>>>           .class = class,
>>>>>>>           .cmd = cmd,
>>>>>>>       };
>>>>>>>
>>>>>>> -    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - 
>>>>>>> sizeof(ctrl));
>>>>>>> +    assert(sizeof(ctrl) < vhost_vdpa_net_cvq_cmd_page_len() -
>>>>>>> +                          (*out_cursor - s->cvq_cmd_out_buffer));
>>>>>>> +    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - 
>>>>>>> sizeof(ctrl) -
>>>>>>> +                       (*out_cursor - s->cvq_cmd_out_buffer));
>>>>>>>
>>>>>>> -    memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>>>>>>> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>>>>>>> +    memcpy(*out_cursor, &ctrl, sizeof(ctrl));
>>>>>>> +    memcpy(*out_cursor + sizeof(ctrl), data, data_size);
>>>>>>>
>>>>>>> -    return vhost_vdpa_net_cvq_add_and_wait(s, sizeof(ctrl) + data_size,
>>>>>>> -                                  sizeof(virtio_net_ctrl_ack));
>>>>>>> +    return vhost_vdpa_net_cvq_add(s, out_cursor, sizeof(ctrl) + 
>>>>>>> data_size,
>>>>>>> +                                  in_cursor, 
>>>>>>> sizeof(virtio_net_ctrl_ack));
>>>>>>>   }
>>>>>>>
>>>>>>> -static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet 
>>>>>>> *n)
>>>>>>> +/**
>>>>>>> + * vhost_vdpa_net_load_mac() restores the NIC mac through SVQ.
>>>>>>> + *
>>>>>>> + * Return the number of elements added to SVQ if success.
>>>>>>> + */
>>>>>>> +static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet 
>>>>>>> *n,
>>>>>>> +                            void **out_cursor, virtio_net_ctrl_ack 
>>>>>>> **in_cursor)
>>>>>>>   {
>>>>>>>       uint64_t features = n->parent_obj.guest_features;
>>>>>>>       if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>>>>>>> -        ssize_t dev_written = vhost_vdpa_net_load_cmd(s, 
>>>>>>> VIRTIO_NET_CTRL_MAC,
>>>>>>> -                                                  
>>>>>>> VIRTIO_NET_CTRL_MAC_ADDR_SET,
>>>>>>> -                                                  n->mac, 
>>>>>>> sizeof(n->mac));
>>>>>>> -        if (unlikely(dev_written < 0)) {
>>>>>>> -            return dev_written;
>>>>>>> -        }
>>>>>>> -
>>>>>>> -        return *s->status != VIRTIO_NET_OK;
>>>>>>> +        return vhost_vdpa_net_load_cmd(s, out_cursor, 
>>>>>>> VIRTIO_NET_CTRL_MAC,
>>>>>>> +                                       VIRTIO_NET_CTRL_MAC_ADDR_SET,
>>>>>>> +                                       n->mac, sizeof(n->mac), 
>>>>>>> in_cursor);
>>>>>>>       }
>>>>>>>
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>
>>>>>>> -static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>>>>>> -                                  const VirtIONet *n)
>>>>>>> +/**
>>>>>>> + * vhost_vdpa_net_load_mac() restores the NIC mq state through SVQ.
>>>>>>> + *
>>>>>>> + * Return the number of elements added to SVQ if success.
>>>>>>> + */
>>>>>>> +static int vhost_vdpa_net_load_mq(VhostVDPAState *s, const VirtIONet 
>>>>>>> *n,
>>>>>>> +                            void **out_cursor, virtio_net_ctrl_ack 
>>>>>>> **in_cursor)
>>>>>>>   {
>>>>>>>       struct virtio_net_ctrl_mq mq;
>>>>>>>       uint64_t features = n->parent_obj.guest_features;
>>>>>>> -    ssize_t dev_written;
>>>>>>>
>>>>>>>       if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
>>>>>>>           return 0;
>>>>>>>       }
>>>>>>>
>>>>>>>       mq.virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs);
>>>>>>> -    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
>>>>>>> -                                          
>>>>>>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &mq,
>>>>>>> -                                          sizeof(mq));
>>>>>>> -    if (unlikely(dev_written < 0)) {
>>>>>>> -        return dev_written;
>>>>>>> -    }
>>>>>>> -
>>>>>>> -    return *s->status != VIRTIO_NET_OK;
>>>>>>> +    return vhost_vdpa_net_load_cmd(s, out_cursor, VIRTIO_NET_CTRL_MQ,
>>>>>>> +                                   VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>>>>>>> +                                   &mq, sizeof(mq), in_cursor);
>>>>>>>   }
>>>>>>>
>>>>>>>   static int vhost_vdpa_net_load(NetClientState *nc)
>>>>>>>   {
>>>>>>>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>>>>>> +    VhostShadowVirtqueue *svq;
>>>>>>> +    void *out_cursor;
>>>>>>> +    virtio_net_ctrl_ack *in_cursor;
>>>>>>>       struct vhost_vdpa *v = &s->vhost_vdpa;
>>>>>>>       const VirtIONet *n;
>>>>>>> -    int r;
>>>>>>> +    ssize_t cmds_in_flight = 0, dev_written, r;
>>>>>>>
>>>>>>>       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>>>>>>>
>>>>>>> @@ -699,14 +750,51 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>>>>>>>       }
>>>>>>>
>>>>>>>       n = VIRTIO_NET(v->dev->vdev);
>>>>>>> -    r = vhost_vdpa_net_load_mac(s, n);
>>>>>>> +    out_cursor = s->cvq_cmd_out_buffer;
>>>>>>> +    in_cursor = s->status;
>>>>>>> +
>>>>>>> +    r = vhost_vdpa_net_load_mac(s, n, &out_cursor, &in_cursor);
>>>>>>>       if (unlikely(r < 0))
>>>>>>>           return r;
>>>>>>>       }
>>>>>>> -    r = vhost_vdpa_net_load_mq(s, n);
>>>>>>> -    if (unlikely(r)) {
>>>>>>> +    cmds_in_flight += r;
>>>>>>> +
>>>>>>> +    r = vhost_vdpa_net_load_mq(s, n, &out_cursor, &in_cursor);
>>>>>>> +    if (unlikely(r < 0)) {
>>>>>>>           return r;
>>>>>>>       }
>>>>>>> +    cmds_in_flight += r;
>>>>>>> +
>>>>>>> +    /* Poll for all used buffer from device */
>>>>>>> +    svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0);
>>>>>>> +    while (cmds_in_flight > 0) {
>>>>>>> +        /*
>>>>>>> +         * We can poll here since we've had BQL from the time we sent 
>>>>>>> the
>>>>>>> +         * descriptor. Also, we need to take the answer before SVQ 
>>>>>>> pulls
>>>>>>> +         * by itself, when BQL is released
>>>>>>> +         */
>>>>>>> +        dev_written = vhost_svq_poll(svq);
>>>>>>
>>>>>> I'd tweak vhost_svq_poll to accept cmds_in_flight.
>>>>
>>>> That sounds great!
>>>> I will refactor the code here and send the v2 patch after
>>>> your patch.
>>>>
>>>> Thanks!
>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>> +
>>>>>>> +        if (unlikely(!dev_written)) {
>>>>>>> +            /*
>>>>>>> +             * vhost_svq_poll() return 0 when something wrong, such as
>>>>>>> +             * QEMU waits for too long time or no available used buffer
>>>>>>> +             * from device, and there is no need to continue polling
>>>>>>> +             * in this case.
>>>>>>> +             */
>>>>>>> +            return -EINVAL;
>>>>>>> +        }
>>>>>>> +
>>>>>>> +        --cmds_in_flight;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* Check the buffers written by device */
>>>>>>> +    for (virtio_net_ctrl_ack *status = s->status; status < in_cursor;
>>>>>>> +         ++status) {
>>>>>>> +        if (*status != VIRTIO_NET_OK) {
>>>>>>> +            return -EINVAL;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>>
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>> --
>>>>>>> 2.25.1
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>