On Wed, Jul 19, 2023 at 9:54 AM Hawkins Jiawei <yin31...@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 <yin31...@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.
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.
Apart from the minor comments I think the series is great, thanks!
> + 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
>
