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

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

 



Sun, Jul 14, 2024 at 04:28:29PM CEST, parav@xxxxxxxxxx wrote:
>
>
>> From: Michael S. Tsirkin <mst@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxx>
>> 
>> 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?

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?



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




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux