Re: [PATCH v2 1/3] virtio_pci_modern: Allow non power of 2 sizes for virtqueues

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Feng Liu <feliu@nvidia.com>
Cc: "virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	Jiri Pirko <jiri@nvidia.com>, Bodong Wang <bodong@nvidia.com>,
	Gavin Li <gavinl@nvidia.com>
Subject: Re: [PATCH v2 1/3] virtio_pci_modern: Allow non power of 2 sizes for virtqueues
Date: Sun, 12 Mar 2023 01:46:32 -0500	[thread overview]
Message-ID: <20230312014544-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <1bdcad80-eca7-9a6b-1375-1ddab3e6bae8@nvidia.com>

On Sat, Mar 11, 2023 at 05:25:04PM -0500, Feng Liu wrote:
> 
> 
> On 2023-03-11 p.m.2:05, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Fri, Mar 10, 2023 at 10:23:16AM -0500, Feng Liu wrote:
> > > 
> > > 
> > > On 2023-03-10 a.m.8:36, Parav Pandit wrote:
> > > > 
> > > > 
> > > > > From: Feng Liu <feliu@nvidia.com>
> > > > > Sent: Friday, March 10, 2023 12:34 AM
> > > > 
> > > > > 
> > > > > - if (!is_power_of_2(num)) {
> > > > > -         dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u",
> > > > > num);
> > > > > -         return ERR_PTR(-EINVAL);
> > > > > - }
> > > > > -
> > > > 
> > > > The check is still valid for split q.
> > > > Maybe the right place for such a check is not the pci transport driver.
> > > > But layer below where split vs packed q knowledge resides and hence, power of 2 check can be omitted for packed vq.
> > > 
> > > Hi, Parav
> > >      I think you are right, I checked the virtio spec, only packed virtqueue
> > > can use queue size which is not power_of_2; so, I think the check can be
> > > reserved only for split queue here, something like
> > > 
> > > struct virtio_device *vdev = &vp_dev->vdev;
> > > if (!virtio_has_feature(vdev, VIRTIO_F_RING_PACKED)
> > >   && !is_power_of_2(num)){
> > >      dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
> > >      return ERR_PTR(-EINVAL);
> > > }
> > > 
> > > I will fix it in next version
> > > 
> > > Hi, Michael and Jason
> > > Do you have any comments on this?
> > > 
> > 
> > Hmm good point. Which raises the question: so how did you test it then?
> > 
> Hi Michael
> 
> I will construct a non power of 2 size packed virtqueue device to test
> whether the driver is loaded successfully and whether the traffic is normal;
> at the same time, I will also construct a non power of 2 size split
> virtqueue device for testing to see if an warning is given and the driver is
> loaded fail
> 
> The method of constructing the device, such as the reply steps in the
> previous email

Okay but previously you said you tested ring sizes 100 and 200 with
iperf. How did you construct these?

> > 
> > --
> > MST
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization