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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 20, 2024 at 4:08 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> 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?

The problem is the spec has bug, see this:

https://www.mail-archive.com/linux-um@xxxxxxxxxxxxxxxxxxx/msg04359.html

Thanks


> 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
>






[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux