Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

Hi Thomas,

I think we have 3 options for this issue.
1) applying this patch;
2) reverting Stephen's original patch;
3) new patch to make both QEMU and GCE work.

1) and 2) will make the test case recover quickly from fail.
As for 3) I don't know whether Stephen has such a patch which can work on both or not.
I don't have GCE environment on hand and I am not an expert on that yet, my current focus is virtio on QEMU,
So at present I have no chance to make a new one to make sure both can work,
But I can help on reviewing if Stephen has a new patch to do that. 

Another thing burst into my thought.
Can we think more about how to setup a mechanism to block those patches which causes critical regression issue?
e.g. this case we are talking about. 
Commit d78deadae4dca240e85054bf2d604a801676becc breaks basic functionality of virtio PMD on QEMU,
It means DPDK sample like vhost, vxlan can't rx any packet and accordingly it can't forward any packet with virtio PMD.
Neither does ovs.

I did review that patch before, but fail to realize it will break the basic function of virtio PMD, it is my bad. 
(Can I send the nack to that patch even after it has been merged into dpdk.org?)
After that, we find that in our testing cycle, we spend time in investigating that and root the cause, and sent out
the fixing patch on July 1.  Keeping virtio basic functionality broken more than 20 days is bad thing for me. 

If we can run a regression automation test with every patch set sent out to dpdk.org, and put those patches breaking any test cases
Into failing-list and notify author, reviewer and maintainer, all those things should be done before theirs being merged, then it will
prevent from merging the erroneous patch into mainline, and thus reduce most reverting patch.

Hi Stephen, and guys in Brocade

Since you nack my patch, then would you pls send out a new patch to fix the issue which your previous patch broke ASAP?
I am not sure you validate your patches on GCE or not, but I strongly suggest you validate each of them on QEMU before
you send out a formal one to dpdk.org.

Hi Qian,
Thanks very much for raising this critical issue in virtio!

thanks,
Changchun


> -----Original Message-----
> From: Xu, Qian Q
> Sent: Monday, July 20, 2015 11:41 AM
> To: Stephen Hemminger; Ouyang, Changchun; 'Thomas Monjalon'
> Cc: dev@dpdk.org; Xu, Qian Q
> Subject: RE: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> Hi, Thomas and all
> I saw in the latest rc1 package, the patch is not merged, and it's a critical issue
> from validation view. I'm responsible for testing the dpdk vhost/virtio
> features, and I found using the latest code, dpdk-vhost/dpdk-virtio can't
> RX/TX package, then my 50% tests are failed while in DPDK2.0 they can pass.
> As you know, it's the basic functions for dpdk virtio to RX/TX, if it's not fixed, I
> think we can't release the R2.1 package. Please help merge the patch, thx.
> 
> 
> 
> Thanks
> Qian
> 
> 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Saturday, July 18, 2015 12:28 AM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> On Wed,  1 Jul 2015 15:48:50 +0800
> Ouyang Changchun <changchun.ouyang@intel.com> wrote:
> 
> > This commit breaks virtio basic packets rx functionality:
> >   d78deadae4dca240e85054bf2d604a801676becc
> >
> > The QEMU use 256 as default vring size, also use this default value to
> > calculate the virtio avail ring base address and used ring base
> > address, and vhost in the backend use the ring base address to do packet
> IO.
> >
> > Virtio spec also says the queue size in PCI configuration is
> > read-only, so virtio front end can't change it. just need use the
> > read-only value to allocate space for vring and calculate the avail
> > and used ring base address. Otherwise, the avail and used ring base
> address will be different between host and guest, accordingly, packet IO
> can't work normally.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c
> > b/drivers/net/virtio/virtio_ethdev.c
> > index fe5f9a1..d84de13 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -263,8 +263,6 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> *dev,
> >  	 */
> >  	vq_size = VIRTIO_READ_REG_2(hw, VIRTIO_PCI_QUEUE_NUM);
> >  	PMD_INIT_LOG(DEBUG, "vq_size: %d nb_desc:%d", vq_size,
> nb_desc);
> > -	if (nb_desc == 0)
> > -		nb_desc = vq_size;
> 
> command queue is setup with nb_desc = 0
> 
> >  	if (vq_size == 0) {
> >  		PMD_INIT_LOG(ERR, "%s: virtqueue does not exist",
> __func__);
> >  		return -EINVAL;
> > @@ -275,15 +273,9 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> *dev,
> >  		return -EINVAL;
> >  	}
> >
> > -	if (nb_desc < vq_size) {
> > -		if (!rte_is_power_of_2(nb_desc)) {
> > -			PMD_INIT_LOG(ERR,
> > -				     "nb_desc(%u) size is not powerof 2",
> > -				     nb_desc);
> > -			return -EINVAL;
> > -		}
> > -		vq_size = nb_desc;
> > -	}
> > +	if (nb_desc != vq_size)
> > +		PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to
> vq size (%d), fall to vq size",
> > +			nb_desc, vq_size);
> 
> Nack. This breaks onn Google Compute Engine the vring size is 16K.
> 
> An application that wants to work on both QEMU and GCE will want to pass a
> reasonable size and have the negotiation resolve to best value.
> 
> For example, vRouter passes 512 as Rx ring size.
> On QEMU this gets rounded down to 256 and on GCE only 512 elements are
> used.
> 
> This is what the Linux kernel virtio does.