From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id DB8293237 for ; Wed, 20 Jul 2016 08:30:42 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 19 Jul 2016 23:30:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,392,1464678000"; d="scan'208";a="1010301886" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.193]) ([10.239.67.193]) by fmsmga001.fm.intel.com with ESMTP; 19 Jul 2016 23:30:41 -0700 To: Yuanhan Liu References: <1468936391-138371-1-git-send-email-jianfeng.tan@intel.com> <20160720043831.GT5146@yliu-dev.sh.intel.com> <9e6084d7-b599-fee2-e575-e3a1ac3fb15b@intel.com> <20160720061325.GU5146@yliu-dev.sh.intel.com> Cc: dev@dpdk.org, zhihong.wang@intel.com, "Xu, Qian Q" From: "Tan, Jianfeng" Message-ID: Date: Wed, 20 Jul 2016 14:30:40 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160720061325.GU5146@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] examples/vhost: fix perf regression X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jul 2016 06:30:43 -0000 On 7/20/2016 2:13 PM, Yuanhan Liu wrote: > On Wed, Jul 20, 2016 at 01:50:34PM +0800, Tan, Jianfeng wrote: >> >> On 7/20/2016 12:38 PM, Yuanhan Liu wrote: >>> On Tue, Jul 19, 2016 at 01:53:11PM +0000, Jianfeng Tan wrote: >>>> We find significant perfermance drop introduced by below commit, >>>> when vhost example is started with --mergeable 0 and inside vm, >>>> kernel virtio-net driver is used to do ip based forwarding. >>>> >>>> The root cause is that below commit adds support for >>>> VIRTIO_NET_F_GUEST_TSO4 and VIRTIO_NET_F_GUEST_TSO6, and when >>>> mergeable is disabled, it triggers big_packets path of virtio-net >>>> driver. In this path, virtio driver uses 19 desc with 18 4K-sized >>>> pages to receive each packet, so that it can receive a big packet >>>> with size of 64K. But QEMU only creates 256 desc entries for each >>>> vq, which results in that only 13 packets can be received. VM >>>> kernel can quickly handle those packets and go to sleep (HLT). >>>> >>>> As QEMU has no option to set the desc entries of a vq, so here, >>>> we disable VIRTIO_NET_F_GUEST_TSO4 and VIRTIO_NET_F_GUEST_TSO6 >>>> with VIRTIO_NET_F_HOST_TSO4 and VIRTIO_NET_F_HOST_TSO6 when we >>>> disable tso of vhost example, to avoid VM kernel virtio driver >>>> go into big_packets path. >>>> >>>> Fixes: 859b480d5afd ("vhost: add guest offload setting") >>> And here you are patching vhost example to try to fix an "issue" >>> in vhost lib, this is __logically__ wrong. >>> >>> --yliu >> This is not an issue from vhost lib's perspective, vhost lib should provide >> all features it supports by default. > Bingo.., that's why "Fixes: 859b480d5afd ... " is wrong to me. > >> Applications can enable/disable >> features according to their own requirements. > Yes, application can, but application normally doesn't do that. And > as stated in my early reply, the qemu is the place you should go for > all those options enabling/disabling, but not vhost (not vhost-example). > > I think it's sometimes more handy if we can do that by introducing > some vhost-example options, and I guess that's why those options are > given. > > In another word, there is nothing wrong about the commit 859b480d5afd, > if you want to "fix" anything here, following commit is something > we need fix: > > Fixes: 9fd72e3cbd29 ("examples/vhost: add virtio offload") > > Because that commit just partially disables some TSO related features, > letting the virtio net driver goes to the slow path. Great, I see. And thanks for detailed clarification. I'll send v2. > >> And the vhost example after >> this commit just triggers a slow path of virtio driver. So this fix just >> makes sure vhost example does not go into the slow path by default. > I have made a statement in the first time, that I am not object to > have this patch at all. > > Meanwhile, the right "fix" is you need disable all TSO related features > from QEMU, in such way, we should see no such issue from all vhost > application, but not only this one, the one we used mostly internally. > > As you can see, it's more about the usage. Yes, I agree this is the BKM we should adopt and recommend users to use. Thanks, Jianfeng > >> By the way, if a fix patch should only involve those commits it will change? > IMO, logically, yes. > > --yliu