Re: [PATCH virtio v2 10/13] virtio_pci_modern: create admin queue of queried size

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 15, 2024 at 09:57:31AM +0200, Jiri Pirko wrote:
> Sun, Jul 14, 2024 at 04:28:29PM CEST, parav@nvidia.com wrote:
> >
> >
> >> From: Michael S. Tsirkin <mst@redhat.com>
> >> Sent: Sunday, July 14, 2024 1:25 PM
> >> 
> >> On Wed, Jul 10, 2024 at 08:35:58AM +0200, Jiri Pirko wrote:
> >> > From: Jiri Pirko <jiri@nvidia.com>
> >> >
> >> > Don't limit the admin queue size to VIRTIO_AVQ_SGS_MAX and rather rely
> >> > on the queried queue size.
> >> >
> >> > Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> I have some doubts about this one. Can't we limit the size to something
> >> sensible? E.g. max number of CPUs? Number of VFs? I don't see why we
> >> should just follow what device did blindly, the device has no idea about use
> >> so would tend to over-provision.
> >>
> >+1.
> >I agree with Michael, we can possibly do min(max_cpus, device_supplied_limit).
> >When more use cases of it arise, this can be improved in future to use larger limit.
> 
> Why max_cpus? How number of cpus is related here?
> 
> regarding max VFs is also value coming from the device,
> why is that better?

I mean the actual # of VFs enabled - that is provided by
software, right?

> For other queues, we also use number provided by the device. Why here
> the behaviour whould be different? Device should know what scale to
> prepare for, why to be smart here?

How can a plug-in device know how it will be used?
For many queues, more is generally better.
But yes, it is quite possible we could do better than
just follow hardware, we just don't have a better idea yet.


> 
> 
> > 
> >> > ---
> >> >  drivers/virtio/virtio_pci_modern.c | 3 +--
> >> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/virtio/virtio_pci_modern.c
> >> > b/drivers/virtio/virtio_pci_modern.c
> >> > index 5ceb4b2c18df..a649c9dc436d 100644
> >> > --- a/drivers/virtio/virtio_pci_modern.c
> >> > +++ b/drivers/virtio/virtio_pci_modern.c
> >> > @@ -550,8 +550,7 @@ static struct virtqueue *setup_vq(struct
> >> virtio_pci_device *vp_dev,
> >> >  	if (index >= vp_modern_get_num_queues(mdev) && !is_avq)
> >> >  		return ERR_PTR(-EINVAL);
> >> >
> >> > -	num = is_avq ?
> >> > -		VIRTIO_AVQ_SGS_MAX : vp_modern_get_queue_size(mdev,
> >> index);
> >> > +	num = vp_modern_get_queue_size(mdev, index);
> >> >  	/* Check if queue is either not available or already active. */
> >> >  	if (!num || vp_modern_get_queue_enable(mdev, index))
> >> >  		return ERR_PTR(-ENOENT);
> >> > --
> >> > 2.45.2
> >