From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Tan, Jianfeng" <jianfeng.tan@intel.com>
Subject: Re: [PATCH] virtio: fix segfault when transmit pkts
Date: Mon, 25 Apr 2016 09:58:23 +0800
Message-ID: <571D79BF.1090604@intel.com>
References: <1461242170-146337-1-git-send-email-jianfeng.tan@intel.com>
 <20160421224444.GC7603@yliu-dev.sh.intel.com>
 <C37D651A908B024F974696C65296B57B4C713546@SHSMSX101.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "dev@dpdk.org" <dev@dpdk.org>,
 Stephen Hemminger <stephen@networkplumber.org>
To: "Xie, Huawei" <huawei.xie@intel.com>,
 Yuanhan Liu <yuanhan.liu@linux.intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
 by dpdk.org (Postfix) with ESMTP id 041B8298F
 for <dev@dpdk.org>; Mon, 25 Apr 2016 03:58:26 +0200 (CEST)
In-Reply-To: <C37D651A908B024F974696C65296B57B4C713546@SHSMSX101.ccr.corp.intel.com>
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>

Hi Yuanhan & Huawei,

On 4/22/2016 10:23 PM, Xie, Huawei wrote:
> On 4/22/2016 6:43 AM, Yuanhan Liu wrote:
>> On Thu, Apr 21, 2016 at 12:36:10PM +0000, Jianfeng Tan wrote:
>>> Issue: when using virtio nic to transmit pkts, it causes segment fault.
>> Jianfeng,
>>
>> It's good to describe the issues, steps to reproduce it and how to fix
>> it. But you don't have to tell it like filling a form. Instead, you
>> could try to describe in plain English.
>>
>>> How to reproduce:
>>> a. start testpmd with vhost.
>>> $testpmd -c 0x3 -n 4 --socket-mem 1024,0 --no-pci \
>>>    --vdev 'eth_vhost0,iface=/tmp/sock0,queues=1' -- -i --nb-cores=1
>> I personally would suggest to add some indentations (and some whitespace
>> lines), like
>>
>> a. start testpmd with vhost.
>>     $ testpmd -c 0x3 -n 4 ... \
>>       --vdev '...' ...
>>
>> b. something else ...
>>     some more explanations.
>>
>> And, do not quote a command line like "$testpmd ...", it's like a var
>> in base in this way. Instead, add a space after "$ ", like what I did
>> above.
>>
>>> b. start a qemu with a virtio nic connected with the vhost-user port.
>> This should be enough. I didn't find any special options below,
>> therefore, above words is enough. However, if it's some specific
>> option introduces a bug, you could claim it aloud then.
>>
>>> $qemu -smp cores=2,sockets=1 -cpu host -enable-kvm vm-0.img -vnc :5 -m 4G \
>>>    -object memory-backend-file,id=mem,size=4096M,mem-path=<path>,share=on \
>>>    -numa node,memdev=mem -mem-prealloc \
>>>    -chardev socket,id=char1,path=$sock_vhost \
>>>    -netdev type=vhost-user,id=net1,chardev=char1 \
>>>    -device virtio-net-pci,netdev=net1,mac=00:01:02:03:04:05
>>> c. enable testpmd on the host.
>>> testpmd> set fwd io
>>> testpmd> start
>>> d. start testpmd in VM.
>>> $testpmd -c 0x3 -n 4 -m 1024 -- -i --disable-hw-vlan-filter --txqflags=0xf01
> With txqflags=0xf01, virtio PMD will use virtio_xmit_pkts_simple, in
> which case the enqueue_xmit willn't called, so typo?

Since mrg_rxbuf is by default enabled, so virtio PMD will not use 
virtio_xmit_pkts_simple.

>
>>> testpmd> set fwd txonly
>>> testpmd> start
>>>
>>> How to fix: this bug is because inside virtqueue_enqueue_xmit(), the flag of
>>> desc has been updated inside the do {} while (); and after the loop, all descs
>>> could have run out, so idx is VQ_RING_DESC_CHAIN_END (32768), use this idx to
>>> reference the start_dp array will lead to segment fault.
>> You missed a fix line here.
> Yes, please include the commit that introduces this bug.

Thanks to both of you for all the advice above. I'll send a v2.

>
>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>> ---
>>>   drivers/net/virtio/virtio_rxtx.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>>> index ef21d8e..432aeab 100644
>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>> @@ -271,8 +271,6 @@ virtqueue_enqueue_xmit(struct virtqueue *txvq, struct rte_mbuf *cookie,
>>>   		idx = start_dp[idx].next;
>>>   	} while ((cookie = cookie->next) != NULL);
>>>   
>>> -	start_dp[idx].flags &= ~VRING_DESC_F_NEXT;
>> This looks a good fix to me. I'm just wondering why we never met it
>> before, and on which specific case do we meet it? Talking about that,
>> seems like "set fwd txonly" is with high suspicious.
> You will not meet this if you have more free descriptors than needed, so
> this depends on the relative speed of virtio xmit and vhost dequeue.
> Also if indirect feature is negotiated, it willn't trigger.
> However, without indirect, seems as long as we start testpmd with txonly
> in guest `first`, we could definitely trigger this. Jianfeng, could you
> confirm this and emphasize the order in the commit message?

Yes, exactly. And will add the scenario description into commit message.

Thanks,
Jianfeng

>
>> 	--yliu
>>
>