From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Xie, Huawei" <huawei.xie@intel.com>
Subject: Re: [PATCH] virtio: fix segfault when transmit pkts
Date: Fri, 22 Apr 2016 14:23:07 +0000
Message-ID: <C37D651A908B024F974696C65296B57B4C713546@SHSMSX101.ccr.corp.intel.com>
References: <1461242170-146337-1-git-send-email-jianfeng.tan@intel.com>
 <20160421224444.GC7603@yliu-dev.sh.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev@dpdk.org" <dev@dpdk.org>, Stephen Hemminger
 <stephen@networkplumber.org>
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>, "Tan, Jianfeng"
 <jianfeng.tan@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by dpdk.org (Postfix) with ESMTP id E6D6E2BE3
 for <dev@dpdk.org>; Fri, 22 Apr 2016 16:23:10 +0200 (CEST)
Content-Language: en-US
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 4/22/2016 6:43 AM, Yuanhan Liu wrote:=0A=
> On Thu, Apr 21, 2016 at 12:36:10PM +0000, Jianfeng Tan wrote:=0A=
>> Issue: when using virtio nic to transmit pkts, it causes segment fault.=
=0A=
> Jianfeng,=0A=
>=0A=
> It's good to describe the issues, steps to reproduce it and how to fix=0A=
> it. But you don't have to tell it like filling a form. Instead, you=0A=
> could try to describe in plain English.=0A=
>=0A=
>> How to reproduce:=0A=
>> a. start testpmd with vhost.=0A=
>> $testpmd -c 0x3 -n 4 --socket-mem 1024,0 --no-pci \=0A=
>>   --vdev 'eth_vhost0,iface=3D/tmp/sock0,queues=3D1' -- -i --nb-cores=3D1=
=0A=
> I personally would suggest to add some indentations (and some whitespace=
=0A=
> lines), like=0A=
>=0A=
> a. start testpmd with vhost.=0A=
>    $ testpmd -c 0x3 -n 4 ... \=0A=
>      --vdev '...' ...=0A=
>=0A=
> b. something else ...=0A=
>    some more explanations.=0A=
>=0A=
> And, do not quote a command line like "$testpmd ...", it's like a var=0A=
> in base in this way. Instead, add a space after "$ ", like what I did=0A=
> above.=0A=
>=0A=
>> b. start a qemu with a virtio nic connected with the vhost-user port.=0A=
> This should be enough. I didn't find any special options below,=0A=
> therefore, above words is enough. However, if it's some specific=0A=
> option introduces a bug, you could claim it aloud then.=0A=
>=0A=
>> $qemu -smp cores=3D2,sockets=3D1 -cpu host -enable-kvm vm-0.img -vnc :5 =
-m 4G \=0A=
>>   -object memory-backend-file,id=3Dmem,size=3D4096M,mem-path=3D<path>,sh=
are=3Don \=0A=
>>   -numa node,memdev=3Dmem -mem-prealloc \=0A=
>>   -chardev socket,id=3Dchar1,path=3D$sock_vhost \=0A=
>>   -netdev type=3Dvhost-user,id=3Dnet1,chardev=3Dchar1 \=0A=
>>   -device virtio-net-pci,netdev=3Dnet1,mac=3D00:01:02:03:04:05=0A=
>> c. enable testpmd on the host.=0A=
>> testpmd> set fwd io=0A=
>> testpmd> start=0A=
>> d. start testpmd in VM.=0A=
>> $testpmd -c 0x3 -n 4 -m 1024 -- -i --disable-hw-vlan-filter --txqflags=
=3D0xf01=0A=
=0A=
With txqflags=3D0xf01, virtio PMD will use virtio_xmit_pkts_simple, in=0A=
which case the enqueue_xmit willn't called, so typo?=0A=
=0A=
>> testpmd> set fwd txonly=0A=
>> testpmd> start=0A=
>>=0A=
>> How to fix: this bug is because inside virtqueue_enqueue_xmit(), the fla=
g of=0A=
>> desc has been updated inside the do {} while (); and after the loop, all=
 descs=0A=
>> could have run out, so idx is VQ_RING_DESC_CHAIN_END (32768), use this i=
dx to=0A=
>> reference the start_dp array will lead to segment fault.=0A=
> You missed a fix line here.=0A=
=0A=
Yes, please include the commit that introduces this bug.=0A=
=0A=
>=0A=
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>=0A=
>> ---=0A=
>>  drivers/net/virtio/virtio_rxtx.c | 2 --=0A=
>>  1 file changed, 2 deletions(-)=0A=
>>=0A=
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virti=
o_rxtx.c=0A=
>> index ef21d8e..432aeab 100644=0A=
>> --- a/drivers/net/virtio/virtio_rxtx.c=0A=
>> +++ b/drivers/net/virtio/virtio_rxtx.c=0A=
>> @@ -271,8 +271,6 @@ virtqueue_enqueue_xmit(struct virtqueue *txvq, struc=
t rte_mbuf *cookie,=0A=
>>  		idx =3D start_dp[idx].next;=0A=
>>  	} while ((cookie =3D cookie->next) !=3D NULL);=0A=
>>  =0A=
>> -	start_dp[idx].flags &=3D ~VRING_DESC_F_NEXT;=0A=
> This looks a good fix to me. I'm just wondering why we never met it=0A=
> before, and on which specific case do we meet it? Talking about that,=0A=
> seems like "set fwd txonly" is with high suspicious.=0A=
=0A=
You will not meet this if you have more free descriptors than needed, so=0A=
this depends on the relative speed of virtio xmit and vhost dequeue.=0A=
Also if indirect feature is negotiated, it willn't trigger.=0A=
However, without indirect, seems as long as we start testpmd with txonly=0A=
in guest `first`, we could definitely trigger this. Jianfeng, could you=0A=
confirm this and emphasize the order in the commit message?=0A=
=0A=
>=0A=
> 	--yliu=0A=
>=0A=
=0A=