[PATCH vhost v9 2/6] virtio: remove support for names array entries being null.

Thu Jun 20 06:51:58 PDT 2024

On Thu, Jun 20, 2024 at 07:04:08PM +0800, Xuan Zhuo wrote:
> On Thu, 20 Jun 2024 07:02:42 -0400, "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 06:49:08PM +0800, Xuan Zhuo wrote:
> > > On Thu, 20 Jun 2024 06:01:54 -0400, "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > > > On Thu, Jun 20, 2024 at 05:04:53PM +0800, Xuan Zhuo wrote:
> > > > > On Thu, 20 Jun 2024 05:01:08 -0400, "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > > > > > On Thu, Jun 20, 2024 at 04:39:38PM +0800, Xuan Zhuo wrote:
> > > > > > > On Thu, 20 Jun 2024 04:02:45 -0400, "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > > > > > > > On Wed, Apr 24, 2024 at 05:15:29PM +0800, Xuan Zhuo wrote:
> > > > > > > > > commit 6457f126c888 ("virtio: support reserved vqs") introduced this
> > > > > > > > > support. Multiqueue virtio-net use 2N as ctrl vq finally, so the logic
> > > > > > > > > doesn't apply. And not one uses this.
> > > > > > > > >
> > > > > > > > > On the other side, that makes some trouble for us to refactor the
> > > > > > > > > find_vqs() params.
> > > > > > > > >
> > > > > > > > > So I remove this support.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
> > > > > > > > > Acked-by: Jason Wang <jasowang at redhat.com>
> > > > > > > > > Acked-by: Eric Farman <farman at linux.ibm.com> # s390
> > > > > > > > > Acked-by: Halil Pasic <pasic at linux.ibm.com>
> > > > > > > >
> > > > > > > >
> > > > > > > > I don't mind, but this patchset is too big already.
> > > > > > > > Why do we need to make this part of this patchset?
> > > > > > >
> > > > > > >
> > > > > > > If some the pointers of the names is NULL, then in the virtio ring,
> > > > > > > we will have a trouble to index from the arrays(names, callbacks...).
> > > > > > > Becasue that the idx of the vq is not the index of these arrays.
> > > > > > >
> > > > > > > If the names is [NULL, "rx", "tx"], the first vq is the "rx", but index of the
> > > > > > > vq is zero, but the index of the info of this vq inside the arrays is 1.
> > > > > >
> > > > > >
> > > > > > Ah. So actually, it used to work.
> > > > > >
> > > > > > What this should refer to is
> > > > > >
> > > > > > commit ddbeac07a39a81d82331a312d0578fab94fccbf1
> > > > > > Author: Wei Wang <wei.w.wang at intel.com>
> > > > > > Date:   Fri Dec 28 10:26:25 2018 +0800
> > > > > >
> > > > > >     virtio_pci: use queue idx instead of array idx to set up the vq
> > > > > >
> > > > > >     When find_vqs, there will be no vq[i] allocation if its corresponding
> > > > > >     names[i] is NULL. For example, the caller may pass in names[i] (i=4)
> > > > > >     with names[2] being NULL because the related feature bit is turned off,
> > > > > >     so technically there are 3 queues on the device, and name[4] should
> > > > > >     correspond to the 3rd queue on the device.
> > > > > >
> > > > > >     So we use queue_idx as the queue index, which is increased only when the
> > > > > >     queue exists.
> > > > > >
> > > > > >     Signed-off-by: Wei Wang <wei.w.wang at intel.com>
> > > > > >     Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > > > > >
> > > > >
> > > > > That just work for PCI.
> > > > >
> > > > > The trouble I described is that we can not index in the virtio ring.
> > > > >
> > > > > In virtio ring, we may like to use the vq.index that do not increase
> > > > > for the NULL.
> > > > >
> > > > >
> > > > > >
> > > > > > Which made it so setting names NULL actually does not reserve a vq.
> > > > > >
> > > > > > But I worry about non pci transports - there's a chance they used
> > > > > > a different index with the balloon. Did you test some of these?
> > > > > >
> > > > >
> > > > > Balloon is out of spec.
> > > > >
> > > > > The vq.index does not increase for the name NULL. So the Balloon use the
> > > > > continuous id. That is out of spec.
> > > >
> > > >
> > > > I see. And apparently the QEMU implementation is out of spec, too,
> > > > so they work fine. And STATS is always on in QEMU.
> > > >
> > > > That change by Wei broke the theoretical config which has
> > > > !STATS but does have FREE_PAGE. We never noticed - not many people
> > > > ever bothered with FREE_PAGE.
> > > >
> > > > However QEMU really is broken in a weird way.
> > > > In particular if it exposes STATS but driver does not
> > > > configure STATS then QEMU still has the stats vq.
> > > > Things will break then.
> > > >
> > > >
> > > > In short, it's a mess, and it needs thought.
> > > > At this point I suggest we keep the ability to set
> > > > names to NULL in case we want to just revert Wei's patch.
> > > >
> > > >
> > > >
> > > > > That does not matter for this patchset.
> > > > > The name NULL is always skipped.
> > > > >
> > > > > Thanks.
> > > >
> > > >
> > > > Let's keep this patchset as small as possible.
> > > > Keep the existing functionality, we'll do cleanups
> > > > later.
> > >
> > >
> > > I am ok. But we need a idx to index the info of the vq.
> > >
> > > How about a new element "cfg_idx" to virtio_vq_config.
> > >
> > > struct virtio_vq_config {
> > > 	unsigned int nvqs;
> > > ->	unsigned int cfg_idx;
> > >
> > > 	struct virtqueue   **vqs;
> > > 	vq_callback_t      **callbacks;
> > > 	const char         **names;
> > > 	const bool          *ctx;
> > > 	struct irq_affinity *desc;
> > > };
> > >
> > >
> > > That is setted by transport. The virtio ring can use this to index the info
> > > of the vq. Then the #1 #2 commits can be dropped.
> > >
> > >
> > > Thanks.
> > >
> >
> > I'm not sure why you need this in the API.
> >
> >
> > Actually now I think about it, the whole struct is weird.
> > I think nvqs etc should be outside the struct.
> > All arrays are the same size, why not:
> >
> > struct virtio_vq_config {
> >  	vq_callback_t      callback;
> >  	const char         *name;
> >  	const bool          ctx;
> > };
> >
> > And find_vqs should get an array of these.
> > Leave the rest of params alone.
> 
> 
> YES, this is great.
> 
> I thought about this.
> 
> The trouble is that all the callers need to be changed.
> That are too many.
> 
> Thanks.
> 

Not too many.

> >
> >
> > >
> > > >
> > > >
> > > > > > --
> > > > > > MST
> > > > > >
> > > >
> >