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

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

[Ouyang Changchun]

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;
 	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);
 
 	if (queue_type == VTNET_RQ) {
 		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
-- 
1.8.4.2

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

[Xu, Qian Q]

Tested-by: Qian Xu <qian.q.xu@intel.com>
- Test Commit: c55e94f560ef5c9fcee4584952de1d0bd414aece
- OS: Fedora 21
- GCC: gcc (GCC) 4.9.2 20141101 (Red Hat 4.9.2-1)
- CPU: Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz
- NIC: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection (rev 01)
- Target: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection (rev 01)
- Total 2 cases(Test Case7 and 8), 2 passed, 0 failed. 

Test Case 1:  test_perf_virtio_one_vm_dpdk_fwd_vhost-cuse_jumboframe
====================================================================

On host:

1. Start up vhost-switch, mergeable 1 means the jubmo frame feature is enabled. vm2vm 0 means only one vm without vm to vm communication::

    taskset -c 1-3 <dpdk_folder>/examples/vhost/build/vhost-switch -c 0xf -n 4 --huge-dir /mnt/huge --socket-mem 1024,1024 -- -p 1 --mergeable 1 --zero-copy 0 --vm2vm 0
   

2. Start VM with vhost cuse as backend::

    taskset -c 4-6  /home/qxu10/qemu-2.2.0/x86_64-softmmu/qemu-system-x86_64 -object memory-backend-file, id=mem,size=2048M,mem-path=/mnt/huge,share=on -numa node,memdev=mem -mem-prealloc \
    -enable-kvm -m 2048 -smp 4 -cpu host -name dpdk1-vm1 \
    -drive file=/home/img/dpdk1-vm1.img \
    -netdev tap,id=vhost3,ifname=tap_vhost3,vhost=on,script=no \
    -device virtio-net pci,netdev=vhost3,mac=52:54:00:00:00:01,id=net3,csum=off,gso=off,guest_csum=off,guest_tso4=off,guest_tso6=off,guest_ecn=off \
    -netdev tap,id=vhost4,ifname=tap_vhost4,vhost=on,script=no \
    -device virtio-net-pci,netdev=vhost4,mac=52:54:00:00:00:02,id=net4,csum=off,gso=off,guest_csum=off,guest_tso4=off,guest_tso6=off,guest_ecn=off \
    -netdev tap,id=ipvm1,ifname=tap3,script=/etc/qemu-ifup -device rtl8139,netdev=ipvm1,id=net0,mac=00:00:00:00:00:01 \
    -localtime -nographic

On guest:

3. ensure the dpdk folder copied to the guest with the same config file and build process as host. Then bind 2 virtio devices to igb_uio and start testpmd, below is the step for reference::

    ./<dpdk_folder>/tools/dpdk_nic_bind.py --bind igb_uio 00:03.0 00:04.0

    ./<dpdk_folder>/x86_64-native-linuxapp-gcc/app/test-pmd/testpmd -c f -n 4 -- -i --txqflags 0x0f00 --max-pkt-len 9000 
    
    $ >set fwd mac
    
    $ >start tx_first

4. After typing start tx_first in testpmd, user can see there would be 2 virtio device with MAC and vlan id registered in vhost-user, the log would be shown in host's vhost-sample output.

5. Send traffic(30second) to virtio1 and virtio2, and set the packet size from 64 to 1518 as well as the jumbo frame 3000. Check the performance in Mpps. The traffic sent to virtio1 should have the DEST MAC of Virtio1's MAC, Vlan id of Virtio1. The traffic sent to virtio2 should have the DEST MAC of Virtio2's MAC, Vlan id of Virtio2. As to the functionality criteria, The received rate should not be zero. As to the performance criteria, need check it with developer or design doc/PRD.

Test Case 7:  test_perf_virtio_one_vm_dpdk_fwd_vhost-user_jumboframe
====================================================================

This case is similar to TestCase1, just change the backend from vhost cuse to vhost-user, so need rebuild the dpdk in vhost-user on host, other steps are same as TestCase1. The command to launch vm is different, see below as example:: 

    <qemu-2.2.0_folder>/x86_64-softmmu/qemu-system-x86_64 -name us-vhost-vm1 -cpu host -enable-kvm -m 2048 -object memory-backend-file,id=mem,size=2048M,mem-path=/mnt/huge,share=on -numa node,memdev=mem -mem-prealloc -smp 2 -drive file=/home/img/dpdk1-vm1.img -chardev socket,id=char0,path=<dpdk/vhost-net -netdev type=vhost-user,id=mynet1,chardev=char0,vhostforce -device virtio-net-pci,mac=52:54:00:00:00:01,netdev=mynet1 -chardev socket,id=char1,path=/home/qxu10/dpdk/vhost-net -netdev type=vhost-user,id=mynet2,chardev=char1,vhostforce -device virtio-net-pci,mac=52:54:00:00:00:02,netdev=mynet2 -netdev tap,id=ipvm1,ifname=tap3,script=/etc/qemu-ifup -device rtl8139,netdev=ipvm1,id=net0,mac=00:00:00:00:00:09 -nographic

On the guest, need add one parameter at the end of testpmd command line: --disable-hw-vlan-filter.

Test Case 8:  test_perf_virtio_one_vm_dpdk_fwd_vhost-user
=========================================================

This case is similar to TestCase7, just set mergeable=0(disable jumbo frame) when launch the vhost sample, and send the packet size from 64B to 1518B to check the performance and basic functions.

Thanks
Qian


-----Original Message-----
From: Ouyang, Changchun 
Sent: Wednesday, July 01, 2015 3:49 PM
To: dev@dpdk.org
Cc: Cao, Waterman; Xu, Qian Q; Ouyang, Changchun
Subject: [PATCH] virtio: fix the vq size issue

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;
 	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);
 
 	if (queue_type == VTNET_RQ) {
 		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
--
1.8.4.2

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

[Xie, Huawei]

On 7/1/2015 3:49 PM, Ouyang Changchun 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.
virtio driver could still use the vq_size to initialize avail ring and
use ring so that they still have the same base address.
The other issue is vhost use  index & (vq->size -1) to index the ring.


Thomas:
This fix works but introduces slight change with original code. Could we
just rollback that commit?

d78deadae4dca240e85054bf2d604a801676becc


>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
>

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

[Ouyang, Changchun]

Hi huawei,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
> Sent: Wednesday, July 1, 2015 11:53 PM
> To: dev@dpdk.org; Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> On 7/1/2015 3:49 PM, Ouyang Changchun 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.
> virtio driver could still use the vq_size to initialize avail ring and use ring so
> that they still have the same base address.
> The other issue is vhost use  index & (vq->size -1) to index the ring.

I am not sure what is your clear message here,
Vhost has no choice but use vq->size -1 to index the ring, 
It is qemu that always use 256 as the vq size, and set the avail and used ring base address,
It also tells vhost the vq size is 256.

> 
> 
> Thomas:
> This fix works but introduces slight change with original code. Could we just
> rollback that commit?
What's your major concern for the slight change here?
just removing the unnecessary check for nb_desc itself.
So I think no issue for the slight change.  

Thanks
Changchun

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

[Xie, Huawei]

On 7/2/2015 8:29 AM, Ouyang, Changchun wrote:
> Hi huawei,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
>> Sent: Wednesday, July 1, 2015 11:53 PM
>> To: dev@dpdk.org; Thomas Monjalon
>> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
>>
>> On 7/1/2015 3:49 PM, Ouyang Changchun 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.
>> virtio driver could still use the vq_size to initialize avail ring and use ring so
>> that they still have the same base address.
>> The other issue is vhost use  index & (vq->size -1) to index the ring.
> I am not sure what is your clear message here,
> Vhost has no choice but use vq->size -1 to index the ring, 
> It is qemu that always use 256 as the vq size, and set the avail and used ring base address,
> It also tells vhost the vq size is 256.

I mean "the same base address issue" could be resolved, but we still
couldn't stop vhost using idx & vq->size -1 to index the ring.

>>
>> Thomas:
>> This fix works but introduces slight change with original code. Could we just
>> rollback that commit?
> What's your major concern for the slight change here?
> just removing the unnecessary check for nb_desc itself.
> So I think no issue for the slight change.  

No major concern. It is better if this patch just rollbacks that commit
without introduce extra change if not necessary.
The original code set nb_desc to vq_size, though it isn't used later.

>
> Thanks
> Changchun
>
>
>
>

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

[Ouyang, Changchun]

> -----Original Message-----
> From: Xie, Huawei
> Sent: Thursday, July 2, 2015 10:02 AM
> To: Ouyang, Changchun; dev@dpdk.org; Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> On 7/2/2015 8:29 AM, Ouyang, Changchun wrote:
> > Hi huawei,
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
> >> Sent: Wednesday, July 1, 2015 11:53 PM
> >> To: dev@dpdk.org; Thomas Monjalon
> >> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> >>
> >> On 7/1/2015 3:49 PM, Ouyang Changchun 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.
> >> virtio driver could still use the vq_size to initialize avail ring
> >> and use ring so that they still have the same base address.
> >> The other issue is vhost use  index & (vq->size -1) to index the ring.
> > I am not sure what is your clear message here, Vhost has no choice but
> > use vq->size -1 to index the ring, It is qemu that always use 256 as
> > the vq size, and set the avail and used ring base address, It also
> > tells vhost the vq size is 256.
> 
> I mean "the same base address issue" could be resolved, but we still couldn't
> stop vhost using idx & vq->size -1 to index the ring.
> 

Then this patch will resolve this avail ring base address issue.

> >>
> >> Thomas:
> >> This fix works but introduces slight change with original code. Could
> >> we just rollback that commit?
> > What's your major concern for the slight change here?
> > just removing the unnecessary check for nb_desc itself.
> > So I think no issue for the slight change.
> 
> No major concern. It is better if this patch just rollbacks that commit without
> introduce extra change if not necessary.
> The original code set nb_desc to vq_size, though it isn't used later.
> 
I prefer to have the slight change to remove unnecessary setting.

> >
> > Thanks
> > Changchun
> >
> >
> >
> >

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

[Xie, Huawei]

On 7/2/2015 10:16 AM, Ouyang, Changchun wrote:
>
>> -----Original Message-----
>> From: Xie, Huawei
>> Sent: Thursday, July 2, 2015 10:02 AM
>> To: Ouyang, Changchun; dev@dpdk.org; Thomas Monjalon
>> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
>>
>> On 7/2/2015 8:29 AM, Ouyang, Changchun wrote:
>>> Hi huawei,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
>>>> Sent: Wednesday, July 1, 2015 11:53 PM
>>>> To: dev@dpdk.org; Thomas Monjalon
>>>> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
>>>>
>>>> On 7/1/2015 3:49 PM, Ouyang Changchun 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.
>>>> virtio driver could still use the vq_size to initialize avail ring
>>>> and use ring so that they still have the same base address.
>>>> The other issue is vhost use  index & (vq->size -1) to index the ring.
>>> I am not sure what is your clear message here, Vhost has no choice but
>>> use vq->size -1 to index the ring, It is qemu that always use 256 as
>>> the vq size, and set the avail and used ring base address, It also
>>> tells vhost the vq size is 256.
>> I mean "the same base address issue" could be resolved, but we still couldn't
>> stop vhost using idx & vq->size -1 to index the ring.
>>
> Then this patch will resolve this avail ring base address issue.
I mean different ring base isn't the root cause. The commit message
which states that this register is read only is simple and enough.

>>>> Thomas:
>>>> This fix works but introduces slight change with original code. Could
>>>> we just rollback that commit?
>>> What's your major concern for the slight change here?
>>> just removing the unnecessary check for nb_desc itself.
>>> So I think no issue for the slight change.
>> No major concern. It is better if this patch just rollbacks that commit without
>> introduce extra change if not necessary.
>> The original code set nb_desc to vq_size, though it isn't used later.
>>
> I prefer to have the slight change to remove unnecessary setting.
>
>>> Thanks
>>> Changchun
>>>
>>>
>>>
>>>
>
>

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

[Ouyang, Changchun]

> -----Original Message-----
> From: Xie, Huawei
> Sent: Thursday, July 2, 2015 5:16 PM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org; Thomas Monjalon
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> On 7/2/2015 10:16 AM, Ouyang, Changchun wrote:
> >
> >> -----Original Message-----
> >> From: Xie, Huawei
> >> Sent: Thursday, July 2, 2015 10:02 AM
> >> To: Ouyang, Changchun; dev@dpdk.org; Thomas Monjalon
> >> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> >>
> >> On 7/2/2015 8:29 AM, Ouyang, Changchun wrote:
> >>> Hi huawei,
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xie, Huawei
> >>>> Sent: Wednesday, July 1, 2015 11:53 PM
> >>>> To: dev@dpdk.org; Thomas Monjalon
> >>>> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> >>>>
> >>>> On 7/1/2015 3:49 PM, Ouyang Changchun 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.
> >>>> virtio driver could still use the vq_size to initialize avail ring
> >>>> and use ring so that they still have the same base address.
> >>>> The other issue is vhost use  index & (vq->size -1) to index the ring.
> >>> I am not sure what is your clear message here, Vhost has no choice
> >>> but use vq->size -1 to index the ring, It is qemu that always use
> >>> 256 as the vq size, and set the avail and used ring base address, It
> >>> also tells vhost the vq size is 256.
> >> I mean "the same base address issue" could be resolved, but we still
> >> couldn't stop vhost using idx & vq->size -1 to index the ring.
> >>
> > Then this patch will resolve this avail ring base address issue.
> I mean different ring base isn't the root cause. The commit message which
> states that this register is read only is simple and enough.	

The direct root cause is avail ring base address issue,
Virtio front end use: vring->avail = vring->desc + vq_size * SIZE_OF_DESC_ELEMENT,
And fill the vring->avail->avail_idx, and the ring itself.
Qemu use:  vring->avail = vring->desc + 256 * SIZE_OF_DESC_ELEMENT,
And tell vhost this address, Vhost use this address to enqueue packets from phy port  into vring.

Pls note that if vq_size is not 256, e.g. it is changed into 128, then the vring->avail in host and in guest
Is totally different, that is why it fail to rx any packet, because they try to use different address to get
Same content in that space.

This is why I still think it is the root cause and I need add it into the commit.

> 
> >>>> Thomas:
> >>>> This fix works but introduces slight change with original code.
> >>>> Could we just rollback that commit?
> >>> What's your major concern for the slight change here?
> >>> just removing the unnecessary check for nb_desc itself.
> >>> So I think no issue for the slight change.
> >> No major concern. It is better if this patch just rollbacks that
> >> commit without introduce extra change if not necessary.
> >> The original code set nb_desc to vq_size, though it isn't used later.
> >>
> > I prefer to have the slight change to remove unnecessary setting.
> >
> >>> Thanks
> >>> Changchun
> >>>
> >>>
> >>>
> >>>
> >
> >

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

[Ouyang, Changchun]

> -----Original Message-----
> From: Ouyang, Changchun
> Sent: Wednesday, July 1, 2015 3:49 PM
> To: dev@dpdk.org
> Cc: Cao, Waterman; Xu, Qian Q; Ouyang, Changchun
> Subject: [PATCH] virtio: fix the vq size issue
> 
> 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;
>  	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);
> 
>  	if (queue_type == VTNET_RQ) {
>  		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
> --
> 1.8.4.2

Any more comments for this patch?

Thanks
Changchun

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

[Xie, Huawei]

Thomas:
Could we roll back that commit or apply Changchun's patch?

/Huawei

On 7/1/2015 11:53 PM, Xie, Huawei wrote:
> On 7/1/2015 3:49 PM, Ouyang Changchun 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.
> virtio driver could still use the vq_size to initialize avail ring and
> use ring so that they still have the same base address.
> The other issue is vhost use  index & (vq->size -1) to index the ring.
>
>
> Thomas:
> This fix works but introduces slight change with original code. Could we
> just rollback that commit?
>
> d78deadae4dca240e85054bf2d604a801676becc
>
>
>> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
>> ---
>>  drivers/net/virtio/virtio_ethdev.c | 14 +++-----------
>>  1 file changed, 3 insertions(+), 11 deletions(-)
>>
>>
>
>

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

[Thomas Monjalon]

2015-07-10 14:05, Xie, Huawei:
> Thomas:
> Could we roll back that commit or apply Changchun's patch?

It is waiting an agreement with Changchun, symbolized by an Acked-by:


> On 7/1/2015 11:53 PM, Xie, Huawei wrote:
> > On 7/1/2015 3:49 PM, Ouyang Changchun 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.
> > virtio driver could still use the vq_size to initialize avail ring and
> > use ring so that they still have the same base address.
> > The other issue is vhost use  index & (vq->size -1) to index the ring.
> >
> >
> > Thomas:
> > This fix works but introduces slight change with original code. Could we
> > just rollback that commit?
> >
> > d78deadae4dca240e85054bf2d604a801676becc
> >
> >
> >> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> >> ---
> >>  drivers/net/virtio/virtio_ethdev.c | 14 +++-----------
> >>  1 file changed, 3 insertions(+), 11 deletions(-)

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

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Friday, July 10, 2015 10:12 PM
> To: Xie, Huawei
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> 2015-07-10 14:05, Xie, Huawei:
> > Thomas:
> > Could we roll back that commit or apply Changchun's patch?
> 
> It is waiting an agreement with Changchun, symbolized by an Acked-by:

I think applying this patch is better than rolling back the previous commit,
Besides fixing the issue, this patch also removes an unnecessary assigning inside that function.

> 
> 
> > On 7/1/2015 11:53 PM, Xie, Huawei wrote:
> > > On 7/1/2015 3:49 PM, Ouyang Changchun 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.
> > > virtio driver could still use the vq_size to initialize avail ring
> > > and use ring so that they still have the same base address.
> > > The other issue is vhost use  index & (vq->size -1) to index the ring.
> > >
> > >
> > > Thomas:
> > > This fix works but introduces slight change with original code.
> > > Could we just rollback that commit?
> > >
> > > d78deadae4dca240e85054bf2d604a801676becc
> > >
> > >
> > >> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > >> ---
> > >>  drivers/net/virtio/virtio_ethdev.c | 14 +++-----------
> > >>  1 file changed, 3 insertions(+), 11 deletions(-)

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

[Thomas Monjalon]

Stephen,

This patch is partially reverting yours:
	http://dpdk.org/browse/dpdk/commit/?id=d78deadae4dca240

A comment or a ack?

2015-07-07 02:32, Ouyang, Changchun:
> 
> > -----Original Message-----
> > From: Ouyang, Changchun
> > Sent: Wednesday, July 1, 2015 3:49 PM
> > To: dev@dpdk.org
> > Cc: Cao, Waterman; Xu, Qian Q; Ouyang, Changchun
> > Subject: [PATCH] virtio: fix the vq size issue
> > 
> > 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;
> >  	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);
> > 
> >  	if (queue_type == VTNET_RQ) {
> >  		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
> > --
> > 1.8.4.2
> 
> Any more comments for this patch?
> 
> Thanks
> Changchun
> 

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

[Stephen Hemminger]

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.

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

[Ouyang, Changchun]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> 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

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.

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

[Xu, Qian Q]

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.

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

[Ouyang, Changchun]

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.

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

[Thomas Monjalon]

2015-07-20 06:18, Ouyang, Changchun:
> 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?

Yes. Non-regression tests are needed. As it must be done with many hardwares
and many configurations, it must be a shared effort.
As a first step, you can run some automated tests by yourself and *manually*
raise errors on the mailing lists. When it will work well, we could discuss
about gathering test reports in a clean distributed way.
Note that this topic is already a work in progress by few people and a public
proposal should be done in few weeks.

> 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 being approved and merged, a nack has no effect.
Having a revert approved is the good way.

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

It wouldn't be so long if these 3 simple things were done:
- use a better title: "virtio: fix Rx from Qemu" instead of a not meaningful "fix the vq size issue"
- cc Stephen (I did it later) who did the original patch you wants to revert
- have an acked-by from Huawei Xie who commented the patch

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

As explained above, it is planned and you can start running you own local test
machine. But please do not spam the mailing list with automated mails from
these tests.

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

[Stephen Hemminger]

On Mon, 20 Jul 2015 06:18:53 +0000
"Ouyang, Changchun" <changchun.ouyang@intel.com> wrote:

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

I am working on #3

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

[Stephen Hemminger]

On Sat, 18 Jul 2015 12:11:11 +0000
"Ouyang, Changchun" <changchun.ouyang@intel.com> wrote:

> Hi Stephen,
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > 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
> 
> 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.

The part in dev_queue_setup is correct, but there is a different problem
if the user has requested smaller number of descriptors. What happens is that
the receive start process runs the mbuf pool out of space getting more packets
than the application expected. Imagine application expects 512 packets in rx ring
but full 16K are allocated.

Working on a fix to the rx initialization logic to take that into account.

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

[Ouyang, Changchun]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, July 20, 2015 6:42 PM
> To: Ouyang, Changchun
> Cc: Xu, Qian Q; Stephen Hemminger; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue
> 
> 2015-07-20 06:18, Ouyang, Changchun:
> > 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?
> 
> Yes. Non-regression tests are needed. As it must be done with many
> hardwares and many configurations, it must be a shared effort.
> As a first step, you can run some automated tests by yourself and
> *manually* raise errors on the mailing lists. When it will work well, we could
> discuss about gathering test reports in a clean distributed way.
> Note that this topic is already a work in progress by few people and a public
> proposal should be done in few weeks.

That's good. 

> 
> > 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 being approved and merged, a nack has no effect.
> Having a revert approved is the good way.

I have acked Stephen's new patch.

> 
> > 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.
> 
> It wouldn't be so long if these 3 simple things were done:
> - use a better title: "virtio: fix Rx from Qemu" instead of a not meaningful "fix
> the vq size issue"
> - cc Stephen (I did it later) who did the original patch you wants to revert
> - have an acked-by from Huawei Xie who commented the patch
> 
> > 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.
> 
> As explained above, it is planned and you can start running you own local test
> machine. But please do not spam the mailing list with automated mails from
> these tests.