[PATCH v2 0/2] Send all the SVQ control commands in parallel

[PATCH v2 0/2] Send all the SVQ control commands in parallel

[Hawkins Jiawei]

This patchset allows QEMU to poll and check the device used buffer
after sending all SVQ control commands, instead of polling and checking
immediately after sending each SVQ control command, so that QEMU can
send all the SVQ control commands in parallel, which have better
performance improvement.

I use vdpa_sim_net to simulate vdpa device, refactor
vhost_vdpa_net_load() to call vhost_vdpa_net_load_mac() 30 times,
to build a test environment for sending multiple SVQ control commands.
The monotonic time to finish vhost_vdpa_net_load() is as follows:

    QEMU                            microseconds
--------------------------------------------------
not patched                              85.092
--------------------------------------------------
patched                                  79.222

So this is a save of (85.092 - 79.222)/30 = 0.2 ms per command.

This patchset resolves the GitLab issue at
https://gitlab.com/qemu-project/qemu/-/issues/1578.

v2:
  - recover accidentally deleted rows
  - remove extra newline
  - refactor `need_poll_len` to `cmds_in_flight`
  - return -EINVAL when vhost_svq_poll() return 0 or check
on buffers written by device fails
  - change the type of `in_cursor`, and refactor the
code for updating cursor
  - return directly when vhost_vdpa_net_load_{mac,mq}()
returns a failure in vhost_vdpa_net_load()

v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-04/msg02668.html

Hawkins Jiawei (2):
  vdpa: rename vhost_vdpa_net_cvq_add()
  vdpa: send CVQ state load commands in parallel

 net/vhost-vdpa.c | 165 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 130 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] vdpa: rename vhost_vdpa_net_cvq_add()

[Hawkins Jiawei]

We want to introduce a new version of vhost_vdpa_net_cvq_add() that
does not poll immediately after forwarding custom buffers
to the device, so that QEMU can send all the SVQ control commands
in parallel instead of serialized.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 net/vhost-vdpa.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 99904a0da7..10804c7200 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -590,8 +590,14 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
     vhost_vdpa_net_client_stop(nc);
 }
 
-static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
-                                      size_t in_len)
+/**
+ * vhost_vdpa_net_cvq_add_and_wait() adds SVQ control commands to SVQ,
+ * kicks the device and polls the device used buffers.
+ *
+ * Return the length written by the device.
+ */
+static ssize_t vhost_vdpa_net_cvq_add_and_wait(VhostVDPAState *s,
+                                    size_t out_len, size_t in_len)
 {
     /* Buffers for the device */
     const struct iovec out = {
@@ -636,7 +642,7 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
     memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
     memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
 
-    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
+    return vhost_vdpa_net_cvq_add_and_wait(s, sizeof(ctrl) + data_size,
                                   sizeof(virtio_net_ctrl_ack));
 }
 
@@ -753,7 +759,8 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
         dev_written = sizeof(status);
         *s->status = VIRTIO_NET_OK;
     } else {
-        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
+        dev_written = vhost_vdpa_net_cvq_add_and_wait(s, out.iov_len,
+                                                      sizeof(status));
         if (unlikely(dev_written < 0)) {
             goto out;
         }
-- 
2.25.1

Re: [PATCH v2 1/2] vdpa: rename vhost_vdpa_net_cvq_add()

[Jason Wang]

On Sat, May 6, 2023 at 10:07 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> We want to introduce a new version of vhost_vdpa_net_cvq_add() that
> does not poll immediately after forwarding custom buffers
> to the device, so that QEMU can send all the SVQ control commands
> in parallel instead of serialized.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
>  net/vhost-vdpa.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 99904a0da7..10804c7200 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -590,8 +590,14 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
>      vhost_vdpa_net_client_stop(nc);
>  }
>
> -static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
> -                                      size_t in_len)
> +/**
> + * vhost_vdpa_net_cvq_add_and_wait() adds SVQ control commands to SVQ,
> + * kicks the device and polls the device used buffers.
> + *
> + * Return the length written by the device.
> + */
> +static ssize_t vhost_vdpa_net_cvq_add_and_wait(VhostVDPAState *s,

Nit: is it better to use "poll" or "sync" other than wait?

Other than this:

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> +                                    size_t out_len, size_t in_len)
>  {
>      /* Buffers for the device */
>      const struct iovec out = {
> @@ -636,7 +642,7 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>      memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>      memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>
> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
> +    return vhost_vdpa_net_cvq_add_and_wait(s, sizeof(ctrl) + data_size,
>                                    sizeof(virtio_net_ctrl_ack));
>  }
>
> @@ -753,7 +759,8 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>          dev_written = sizeof(status);
>          *s->status = VIRTIO_NET_OK;
>      } else {
> -        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> +        dev_written = vhost_vdpa_net_cvq_add_and_wait(s, out.iov_len,
> +                                                      sizeof(status));
>          if (unlikely(dev_written < 0)) {
>              goto out;
>          }
> --
> 2.25.1
>

Re: [PATCH v2 1/2] vdpa: rename vhost_vdpa_net_cvq_add()

[Hawkins Jiawei]

Sorry for forgetting cc when replying to the email.
I will resend this email with cc.

On Wed, 17 May 2023 at 12:12, Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, May 6, 2023 at 10:07 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >
> > We want to introduce a new version of vhost_vdpa_net_cvq_add() that
> > does not poll immediately after forwarding custom buffers
> > to the device, so that QEMU can send all the SVQ control commands
> > in parallel instead of serialized.
> >
> > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> > ---
> >  net/vhost-vdpa.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 99904a0da7..10804c7200 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -590,8 +590,14 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
> >      vhost_vdpa_net_client_stop(nc);
> >  }
> >
> > -static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
> > -                                      size_t in_len)
> > +/**
> > + * vhost_vdpa_net_cvq_add_and_wait() adds SVQ control commands to SVQ,
> > + * kicks the device and polls the device used buffers.
> > + *
> > + * Return the length written by the device.
> > + */
> > +static ssize_t vhost_vdpa_net_cvq_add_and_wait(VhostVDPAState *s,
>
> Nit: is it better to use "poll" or "sync" other than wait?
>
> Other than this:
>
> Acked-by: Jason Wang <jasowang@redhat.com>

Hi Jason,

Thanks for your suggestion. I prefer 'poll', which makes it clearer
that this function will poll immediately compared to the new
version of vhost_vdpa_net_cvq_add().

I will refactor this in the v2 patch with the Acked-by tag on.

Thanks!

>
> Thanks
>
> > +                                    size_t out_len, size_t in_len)
> >  {
> >      /* Buffers for the device */
> >      const struct iovec out = {
> > @@ -636,7 +642,7 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
> >      memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
> >      memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
> >
> > -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
> > +    return vhost_vdpa_net_cvq_add_and_wait(s, sizeof(ctrl) + data_size,
> >                                    sizeof(virtio_net_ctrl_ack));
> >  }
> >
> > @@ -753,7 +759,8 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> >          dev_written = sizeof(status);
> >          *s->status = VIRTIO_NET_OK;
> >      } else {
> > -        dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
> > +        dev_written = vhost_vdpa_net_cvq_add_and_wait(s, out.iov_len,
> > +                                                      sizeof(status));
> >          if (unlikely(dev_written < 0)) {
> >              goto out;
> >          }
> > --
> > 2.25.1
> >
>

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

[Hawkins Jiawei]

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,
+                                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);
+
+        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

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

[Jason Wang]

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 ?

> +                                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.

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
>

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

[Eugenio Perez Martin]

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?

Maybe we can create a struct for them and then pass just this struct?

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.
>
> 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
> >
>

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

[Hawkins Jiawei]

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?
>
> 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
> > >
> >
>

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

[Jason Wang]

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.

Or it would be even better to have some buffer allocation helpers to
alloc and free in and out buffers.

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
> > > >
> > >
> >
>

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

[Eugenio Perez Martin]

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.

> 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.

To manage them dynamically would need code to map them at any time etc.

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
> > > > >
> > > >
> > >
> >
>

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

[Jason Wang]

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.
>
> > 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
> > > > > >
> > > > >
> > > >
> > >
> >
>

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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>