On 2023/5/18 14:12, Jason Wang wrote: > On Thu, May 18, 2023 at 2:00 PM Eugenio Perez Martin > <epere...@redhat.com> wrote: >> >> On Thu, May 18, 2023 at 7:47 AM Jason Wang <jasow...@redhat.com> wrote: >>> >>> On Wed, May 17, 2023 at 11:02 PM Hawkins Jiawei <yin31...@gmail.com> wrote: >>>> >>>> Sorry for forgetting cc when replying to the email. >>>> >>>> On Wed, 17 May 2023 at 16:22, Eugenio Perez Martin <epere...@redhat.com> >>>> wrote: >>>>> >>>>> On Wed, May 17, 2023 at 7:22 AM Jason Wang <jasow...@redhat.com> wrote: >>>>>> >>>>>> On Sat, May 6, 2023 at 10:07 PM Hawkins Jiawei <yin31...@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 <yin31...@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 >>>>>>> >>>>>> >>>>> >>>> >>> >> >