On Thursday, June 20, 2024 5:01 PM, Michael S. Tsirkin 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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxx>
> > > > Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
> > > > Acked-by: Eric Farman <farman@xxxxxxxxxxxxx> # s390
> > > > Acked-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
> > >
> > >
> > > 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
The approach was taken to prevent the creation (by the device) of unnecessary
queues that would remain unused when the feature bit is turned off. Otherwise,
the device is required to create all conditional queues regardless of their necessity.
>
> Which made it so setting names NULL actually does not reserve a vq.
If there is a need for an explicit queue reservation, it might be feasible to assign
a specific name to the queue(e.g. "reserved")?
This will require the device to have the reserved queue added.
>
> 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?
>
> --
> MST
>
