On Wed, Apr 24, 2024 at 05:15:28PM +0800, 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.
>
> As specification 1.0-1.2, vq indexes should not be contiguous if some
> vq does not exist. But currently the virtqueue index is contiguous for
> all existing devices. The Linux kernel does not implement functionality
> to allow vq indexes to be discontinuous. So the current behavior of the
> virtio-balloon device is different for the spec. But this commit has no
> functional changes.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>
> Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
I can't make heads of tails of this.
David you acked so maybe you can help rewrite the commit log here?
I don't understand what this says.
What in the balloon driver is out of spec?
NULL in names *exactly* allows skipping init for some vqs.
How is that "does not implement"?
And so on.
> ---
> drivers/virtio/virtio_balloon.c | 48 ++++++++++++++-------------------
> 1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index c0a63638f95e..ccda6d08493f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -548,49 +548,41 @@ 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, idx = 0;
>
> - /*
> - * 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";
> - 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;
> + callbacks[idx] = balloon_ack;
> + names[idx++] = "inflate";
> + callbacks[idx] = balloon_ack;
> + names[idx++] = "deflate";
>
> 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[idx] = "stats";
> + callbacks[idx++] = stats_request;
> }
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> - names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
> - callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> + names[idx] = "free_page_vq";
> + callbacks[idx++] = NULL;
> }
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)) {
> - names[VIRTIO_BALLOON_VQ_REPORTING] = "reporting_vq";
> - callbacks[VIRTIO_BALLOON_VQ_REPORTING] = balloon_ack;
> + names[idx] = "reporting_vq";
> + callbacks[idx++] = balloon_ack;
> }
>
> - err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs,
> - callbacks, names, NULL);
> + err = virtio_find_vqs(vb->vdev, idx, vqs, callbacks, names, NULL);
> if (err)
> return err;
>
> - vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
> - vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
> + idx = 0;
> +
> + vb->inflate_vq = vqs[idx++];
> + vb->deflate_vq = vqs[idx++];
> +
> 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
> @@ -610,10 +602,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++];
>
> return 0;
> }
> --
> 2.32.0.3.g01195cf9f
