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