Re: [PATCH v4 3/8] vhost: Expose vhost_svq_available_slots()

[Eugenio Perez Martin]

On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> Next patches in this series will delay the polling
> and checking of buffers until either the SVQ is
> full or control commands shadow buffers are full,
> no longer perform an immediate poll and check of
> the device's used buffers for each CVQ state load command.
>
> To achieve this, this patch exposes
> vhost_svq_available_slots() and introduces a helper function,
> allowing QEMU to know whether the SVQ is full.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 2 +-
>  hw/virtio/vhost-shadow-virtqueue.h | 1 +
>  net/vhost-vdpa.c                   | 9 +++++++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> b/hw/virtio/vhost-shadow-virtqueue.c
> index e731b1d2ea..fc5f408f77 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -66,7 +66,7 @@ bool vhost_svq_valid_features(uint64_t features, Error 
> **errp)
>   *
>   * @svq: The svq
>   */
> -static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq)
> +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq)
>  {
>      return svq->num_free;
>  }
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> b/hw/virtio/vhost-shadow-virtqueue.h
> index 5bce67837b..19c842a15b 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -114,6 +114,7 @@ typedef struct VhostShadowVirtqueue {
>
>  bool vhost_svq_valid_features(uint64_t features, Error **errp);
>
> +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq);
>  void vhost_svq_push_elem(VhostShadowVirtqueue *svq,
>                           const VirtQueueElement *elem, uint32_t len);
>  int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,

I think it is ok to split this export in its own patch. If you decide
to do it that way, you can add my Acked-by.

> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index a875767ee9..e6342b213f 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -620,6 +620,13 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s,
>      return vhost_svq_poll(svq, 1);
>  }
>
> +/* Convenience wrapper to get number of available SVQ descriptors */
> +static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s)
> +{
> +    VhostShadowVirtqueue *svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 
> 0);

This is not really generic enough for all VhostVDPAState, as dataplane
ones have two svqs.

I think the best is to just inline the function in the caller, as
there is only one, isn't it? If not, would it work to just replace
_net_ by _cvq_ or similar?

> +    return vhost_svq_available_slots(svq);
> +}
> +
>  static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>                                         uint8_t cmd, const struct iovec 
> *data_sg,
>                                         size_t data_num)
> @@ -640,6 +647,8 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, 
> uint8_t class,
>      };
>
>      assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
> +    /* Each CVQ command has one out descriptor and one in descriptor */
> +    assert(vhost_vdpa_net_svq_available_slots(s) >= 2);
>

I think we should remove this assertion. By the end of the series
there is an "if" checks explicitly for the opposite condition, and
flushing the queue in that case, so the code can never reach it.

>      /* pack the CVQ command header */
>      memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
> --
> 2.25.1
>