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

[Ouyang, Changchun]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Saturday, July 18, 2015 12:28 AM
> To: Ouyang, Changchun
> Cc: dev at 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 at 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 at 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

nb_desc is not used in the rest of the function, then why we need such an assignment here?
Why command queues is setup whit nb_desc = 0?
Even if it is the case, what the code change break? 

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


As I mentioned before, the commit d78deadae4dca240e85054bf2d604a801676becc break the basic functionality of virtio pmd,
I don't think keeping it broken is good way for us.
We have to revert it firstly to recover its functionality on qemu!
Why we need break current functionality to just meet a new thing's requirement?

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

Do you have already a patch to revert the mistaken and support both qemu and gce?
If you have, then pls send out it, and let's review.

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