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

[Eugenio Perez Martin <eperezma@redhat.com>]

On Tue, Aug 29, 2023 at 7:55 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>
> ---
> v4:
>   - refactor argument `cmds_in_flight` to `len` for
> vhost_vdpa_net_svq_full()
>   - check the return value of vhost_vdpa_net_svq_poll()
> in vhost_vdpa_net_svq_flush() suggested by Eugenio
>   - use iov_size(), vhost_vdpa_net_load_cursor_reset()
> and iov_discard_front() to update the cursors instead of
> accessing it directly according to Eugenio
>
> v3: https://lore.kernel.org/all/3a002790e6c880af928c6470ecbf03e7c65a68bb.1689748694.git.yin31149@gmail.com/
>
>  net/vhost-vdpa.c | 155 +++++++++++++++++++++++++++++------------------
>  1 file changed, 97 insertions(+), 58 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index a71e8c9090..818464b702 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -646,6 +646,31 @@ static void vhost_vdpa_net_load_cursor_reset(VhostVDPAState *s,
>      in_cursor->iov_len = vhost_vdpa_net_cvq_cmd_page_len();
>  }
>
> +/*
> + * Poll SVQ for multiple pending control commands and check the device's ack.
> + *
> + * Caller should hold the BQL when invoking this function.
> + *
> + * @s: The VhostVDPAState
> + * @len: The length of the pending status shadow buffer
> + */
> +static ssize_t vhost_vdpa_net_svq_flush(VhostVDPAState *s, size_t len)
> +{
> +    /* Device uses a one-byte length ack for each control command */
> +    ssize_t dev_written = vhost_vdpa_net_svq_poll(s, len);
> +    if (unlikely(dev_written != len)) {
> +        return -EIO;
> +    }
> +
> +    /* check the device's ack */
> +    for (int i = 0; i < len; ++i) {
> +        if (s->status[i] != VIRTIO_NET_OK) {
> +            return -EIO;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s,
>                                         struct iovec *out_cursor,
>                                         struct iovec *in_cursor, uint8_t class,
> @@ -660,10 +685,30 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s,
>             cmd_size = sizeof(ctrl) + data_size;
>      struct iovec out, in;
>      ssize_t r;
> +    unsigned dummy_cursor_iov_cnt;
>
>      assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
> +    if (vhost_vdpa_net_svq_available_slots(s) < 2 ||
> +        iov_size(out_cursor, 1) < cmd_size) {
> +        /*
> +         * 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->iov_base -
> +                                     (void *)s->status);
> +        if (unlikely(r < 0)) {
> +            return r;
> +        }
> +
> +        vhost_vdpa_net_load_cursor_reset(s, out_cursor, in_cursor);
> +    }
> +

It would be great to merge this flush with the one at
vhost_vdpa_net_load. We would need to return ENOSPC or similar and
handle it there.

But it would make it more difficult to iterate through the loading of
the different parameters, so I think it can be done on top.

>      /* Each CVQ command has one out descriptor and one in descriptor */
>      assert(vhost_vdpa_net_svq_available_slots(s) >= 2);
> +    assert(iov_size(out_cursor, 1) >= cmd_size);
>

Same here, I think we can avoid the assertion, right?

Apart from that,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

>      /* Prepare the buffer for out descriptor for the device */
>      iov_copy(&out, 1, out_cursor, 1, 0, cmd_size);
> @@ -681,11 +726,13 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s,
>          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 */
> +    dummy_cursor_iov_cnt = 1;
> +    iov_discard_front(&out_cursor, &dummy_cursor_iov_cnt, cmd_size);
> +    dummy_cursor_iov_cnt = 1;
> +    iov_discard_front(&in_cursor, &dummy_cursor_iov_cnt, sizeof(*s->status));
> +
> +    return 0;
>  }
>
>  static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
> @@ -697,15 +744,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;
>          }
>      }
>
> @@ -750,15 +794,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;
> @@ -770,7 +811,7 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>                                    struct iovec *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;
> @@ -781,15 +822,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;
> @@ -801,7 +839,7 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>                                          struct iovec *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)) {
> @@ -829,15 +867,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;
> @@ -853,16 +888,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;
> @@ -1019,15 +1050,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;
> @@ -1096,6 +1124,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.iov_base - (void *)s->status);
> +    if (unlikely(r)) {
> +        return r;
> +    }
> +
>      return 0;
>  }
>
> --
> 2.25.1
>