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

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Fri, 22 Mar 2024 12:56:46 +0100, David Hildenbrand <david@redhat.com> wrote:
> 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

Will fix in next version.

Thanks.


>
> --
> Cheers,
>
> David / dhildenb
>