Re: [PATCH v3 8/8] vdpa: Send cvq state load commands in parallel

[Hawkins Jiawei <yin31149@gmail.com>]

On 2023/8/19 01:27, Eugenio Perez Martin wrote:
> On Wed, Jul 19, 2023 at 9:54 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> This patch enables sending CVQ state load commands
>> in parallel at device startup by following steps:
>>
>>    * Refactor vhost_vdpa_net_load_cmd() to iterate through
>> the control commands shadow buffers. This allows different
>> CVQ state load commands to use their own unique buffers.
>>
>>    * Delay the polling and checking of buffers until either
>> the SVQ is full or control commands shadow buffers are full.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1578
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>>   net/vhost-vdpa.c | 157 +++++++++++++++++++++++++++++------------------
>>   1 file changed, 96 insertions(+), 61 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 795c9c1fd2..1ebb58f7f6 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -633,6 +633,26 @@ static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s)
>>       return vhost_svq_available_slots(svq);
>>   }
>>
>> +/*
>> + * Poll SVQ for multiple pending control commands and check the device's ack.
>> + *
>> + * Caller should hold the BQL when invoking this function.
>> + */
>> +static ssize_t vhost_vdpa_net_svq_flush(VhostVDPAState *s,
>> +                                        size_t cmds_in_flight)
>> +{
>> +    vhost_vdpa_net_svq_poll(s, cmds_in_flight);
>> +
>> +    /* Device should and must use only one byte ack each control command */
>> +    assert(cmds_in_flight < vhost_vdpa_net_cvq_cmd_page_len());
>> +    for (int i = 0; i < cmds_in_flight; ++i) {
>> +        if (s->status[i] != VIRTIO_NET_OK) {
>> +            return -EIO;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>>   static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, void **out_cursor,
>>                                          void **in_cursor, uint8_t class,
>>                                          uint8_t cmd, const struct iovec *data_sg,
>> @@ -642,19 +662,41 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, void **out_cursor,
>>           .class = class,
>>           .cmd = cmd,
>>       };
>> -    size_t data_size = iov_size(data_sg, data_num);
>> +    size_t data_size = iov_size(data_sg, data_num),
>> +           left_bytes = vhost_vdpa_net_cvq_cmd_page_len() -
>> +                        (*out_cursor - s->cvq_cmd_out_buffer);
>>       /* Buffers for the device */
>>       struct iovec out = {
>> -        .iov_base = *out_cursor,
>>           .iov_len = sizeof(ctrl) + data_size,
>>       };
>>       struct iovec in = {
>> -        .iov_base = *in_cursor,
>>           .iov_len = sizeof(*s->status),
>>       };
>>       ssize_t r;
>>
>> -    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>> +    if (sizeof(ctrl) > left_bytes || data_size > left_bytes - sizeof(ctrl) ||
>
> I'm ok with this code, but maybe we can simplify the code if we use
> two struct iovec as cursors instead of a void **? I think functions
> like iov_size and iov_copy already take care of a few checks here.

Hi Eugenio,

Thanks for the explanation, I will refactor the patch according to your
suggestion!

>
> Apart from that it would be great to merge this call to
> vhost_vdpa_net_svq_flush, but I find it very hard to do unless we
> scatter it through all callers of vhost_vdpa_net_load_cmd.

Yes, I agree with you. Maybe we can consider refactoring like this in
the future if needed.

>
> Apart from the minor comments I think the series is great, thanks!

Thanks for your review:)!


>
>> +        vhost_vdpa_net_svq_available_slots(s) < 2) {
>> +        /*
>> +         * It is time to flush all pending control commands if SVQ is full
>> +         * or control commands shadow buffers are full.
>> +         *
>> +         * We can poll here since we've had BQL from the time
>> +         * we sent the descriptor.
>> +         */
>> +        r = vhost_vdpa_net_svq_flush(s, *in_cursor - (void *)s->status);
>> +        if (unlikely(r < 0)) {
>> +            return r;
>> +        }
>> +
>> +        *out_cursor = s->cvq_cmd_out_buffer;
>> +        *in_cursor = s->status;
>> +        left_bytes = vhost_vdpa_net_cvq_cmd_page_len();
>> +    }
>> +
>> +    out.iov_base = *out_cursor;
>> +    in.iov_base = *in_cursor;
>> +
>> +    assert(data_size <= left_bytes - sizeof(ctrl));
>>       /* Each CVQ command has one out descriptor and one in descriptor */
>>       assert(vhost_vdpa_net_svq_available_slots(s) >= 2);
>>
>> @@ -670,11 +712,11 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, void **out_cursor,
>>           return r;
>>       }
>>
>> -    /*
>> -     * We can poll here since we've had BQL from the time
>> -     * we sent the descriptor.
>> -     */
>> -    return vhost_vdpa_net_svq_poll(s, 1);
>> +    /* iterate the cursors */
>> +    *out_cursor += out.iov_len;
>> +    *in_cursor += in.iov_len;
>> +
>> +    return 0;
>>   }
>>
>>   static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
>> @@ -685,15 +727,12 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
>>               .iov_base = (void *)n->mac,
>>               .iov_len = sizeof(n->mac),
>>           };
>> -        ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> -                                                  VIRTIO_NET_CTRL_MAC,
>> -                                                  VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> -                                                  &data, 1);
>> -        if (unlikely(dev_written < 0)) {
>> -            return dev_written;
>> -        }
>> -        if (*s->status != VIRTIO_NET_OK) {
>> -            return -EIO;
>> +        ssize_t r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +                                               VIRTIO_NET_CTRL_MAC,
>> +                                               VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> +                                               &data, 1);
>> +        if (unlikely(r < 0)) {
>> +            return r;
>>           }
>>       }
>>
>> @@ -738,15 +777,12 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
>>               .iov_len = mul_macs_size,
>>           },
>>       };
>> -    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +    ssize_t r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>>                                   VIRTIO_NET_CTRL_MAC,
>>                                   VIRTIO_NET_CTRL_MAC_TABLE_SET,
>>                                   data, ARRAY_SIZE(data));
>> -    if (unlikely(dev_written < 0)) {
>> -        return dev_written;
>> -    }
>> -    if (*s->status != VIRTIO_NET_OK) {
>> -        return -EIO;
>> +    if (unlikely(r < 0)) {
>> +        return r;
>>       }
>>
>>       return 0;
>> @@ -757,7 +793,7 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>                                     void **out_cursor, void **in_cursor)
>>   {
>>       struct virtio_net_ctrl_mq mq;
>> -    ssize_t dev_written;
>> +    ssize_t r;
>>
>>       if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_MQ)) {
>>           return 0;
>> @@ -768,15 +804,12 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>           .iov_base = &mq,
>>           .iov_len = sizeof(mq),
>>       };
>> -    dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> -                                          VIRTIO_NET_CTRL_MQ,
>> -                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>> -                                          &data, 1);
>> -    if (unlikely(dev_written < 0)) {
>> -        return dev_written;
>> -    }
>> -    if (*s->status != VIRTIO_NET_OK) {
>> -        return -EIO;
>> +    r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +                                   VIRTIO_NET_CTRL_MQ,
>> +                                   VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>> +                                   &data, 1);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>>       }
>>
>>       return 0;
>> @@ -787,7 +820,7 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>>                                           void **out_cursor, void **in_cursor)
>>   {
>>       uint64_t offloads;
>> -    ssize_t dev_written;
>> +    ssize_t r;
>>
>>       if (!virtio_vdev_has_feature(&n->parent_obj,
>>                                    VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
>> @@ -815,15 +848,12 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>>           .iov_base = &offloads,
>>           .iov_len = sizeof(offloads),
>>       };
>> -    dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> -                                          VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>> -                                          VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>> -                                          &data, 1);
>> -    if (unlikely(dev_written < 0)) {
>> -        return dev_written;
>> -    }
>> -    if (*s->status != VIRTIO_NET_OK) {
>> -        return -EIO;
>> +    r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +                                   VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>> +                                   VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>> +                                   &data, 1);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>>       }
>>
>>       return 0;
>> @@ -838,15 +868,12 @@ static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>>           .iov_base = &on,
>>           .iov_len = sizeof(on),
>>       };
>> -    ssize_t dev_written;
>> +    ssize_t r;
>>
>> -    dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> -                                          VIRTIO_NET_CTRL_RX, cmd, &data, 1);
>> -    if (unlikely(dev_written < 0)) {
>> -        return dev_written;
>> -    }
>> -    if (*s->status != VIRTIO_NET_OK) {
>> -        return -EIO;
>> +    r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +                                VIRTIO_NET_CTRL_RX, cmd, &data, 1);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>>       }
>>
>>       return 0;
>> @@ -1001,15 +1028,12 @@ static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>>           .iov_base = &vid,
>>           .iov_len = sizeof(vid),
>>       };
>> -    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> -                                                  VIRTIO_NET_CTRL_VLAN,
>> -                                                  VIRTIO_NET_CTRL_VLAN_ADD,
>> -                                                  &data, 1);
>> -    if (unlikely(dev_written < 0)) {
>> -        return dev_written;
>> -    }
>> -    if (unlikely(*s->status != VIRTIO_NET_OK)) {
>> -        return -EIO;
>> +    ssize_t r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +                                           VIRTIO_NET_CTRL_VLAN,
>> +                                           VIRTIO_NET_CTRL_VLAN_ADD,
>> +                                           &data, 1);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>>       }
>>
>>       return 0;
>> @@ -1078,6 +1102,17 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>>           return r;
>>       }
>>
>> +    /*
>> +     * We need to poll and check all pending device's used buffers.
>> +     *
>> +     * We can poll here since we've had BQL from the time
>> +     * we sent the descriptor.
>> +     */
>> +    r = vhost_vdpa_net_svq_flush(s, in_cursor - (void *)s->status);
>> +    if (unlikely(r)) {
>> +        return r;
>> +    }
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.25.1
>>
>