From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id D86C53237 for ; Wed, 20 Jul 2016 08:09:30 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 19 Jul 2016 23:09:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,392,1464678000"; d="scan'208";a="737026754" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by FMSMGA003.fm.intel.com with ESMTP; 19 Jul 2016 23:09:30 -0700 Date: Wed, 20 Jul 2016 14:13:25 +0800 From: Yuanhan Liu To: "Tan, Jianfeng" Cc: dev@dpdk.org, zhihong.wang@intel.com, "Xu, Qian Q" Message-ID: <20160720061325.GU5146@yliu-dev.sh.intel.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9e6084d7-b599-fee2-e575-e3a1ac3fb15b@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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:09:31 -0000 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. > 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. > By the way, if a fix patch should only involve those commits it will change? IMO, logically, yes. --yliu