Re: [PATCH vhost v4 1/6] virtio_balloon: remove the dependence where names[] is null

David Hildenbrand
2024-03-22T11:56:54Z

On 21.03.24 11:15, Xuan Zhuo wrote:
> Currently, the init_vqs function within the virtio_balloon driver relies
> on the condition that certain names array entries are null in order to
> skip the initialization of some virtual queues (vqs). This behavior is
> unique to this part of the codebase. In an upcoming commit, we plan to
> eliminate this dependency by removing the function entirely. Therefore,
> with this change, we are ensuring that the virtio_balloon no longer
> depends on the aforementioned function.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_balloon.c | 41 +++++++++++++++------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 1f5b3dd31fcf..becc12a05407 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -531,49 +531,46 @@ static int init_vqs(struct virtio_balloon *vb)
> struct virtqueue *vqs[VIRTIO_BALLOON_VQ_MAX];
> vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX];
> const char *names[VIRTIO_BALLOON_VQ_MAX];
> - int err;
> + int err, nvqs, idx;
>
> - /*
> - * Inflateq and deflateq are used unconditionally. The names[]
> - * will be NULL if the related feature is not enabled, which will
> - * cause no allocation for the corresponding virtqueue in find_vqs.
> - */
> callbacks[VIRTIO_BALLOON_VQ_INFLATE] = balloon_ack;
> names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";

I'd remove the static dependencies here completely and do it
consistently:

nvqs = 0;

callbacks[nvqs] = balloon_ack;
names[nvqs++] = "inflate";
callbacks[nvqs] = balloon_ack;
names[nvqs++] = "deflate";

> - callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL;
> - names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> - callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> - names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> - names[VIRTIO_BALLOON_VQ_REPORTING] = NULL;
> +
> + nvqs = VIRTIO_BALLOON_VQ_DEFLATE + 1;
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> - names[VIRTIO_BALLOON_VQ_STATS] = "stats";
> - callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request;
> + names[nvqs] = "stats";
> + callbacks[nvqs] = stats_request;
> + ++nvqs;

callbacks[nvqs++] = stats_request;

If you prefer to keep it separate, "nvqs++" please.

... same here:

idx = 0;
vb->inflate_vq = vqs[idx++];
vb->deflate_vq = vqs[idx++];

...

>
> vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
> vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
> +
> + idx = VIRTIO_BALLOON_VQ_DEFLATE + 1;
> +
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> struct scatterlist sg;
> unsigned int num_stats;
> - vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS];
> + vb->stats_vq = vqs[idx++];
>
> /*
> * Prime this virtqueue with one buffer so the hypervisor can
> @@ -593,10 +590,10 @@ static int init_vqs(struct virtio_balloon *vb)
> }
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> - vb->free_page_vq = vqs[VIRTIO_BALLOON_VQ_FREE_PAGE];
> + vb->free_page_vq = vqs[idx++];
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING))
> - vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING];
> + vb->reporting_vq = vqs[idx++];
>

Apart from that LGTM

--
Cheers,

David / dhildenb