From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 8C15A58F7 for ; Wed, 17 Aug 2016 08:42:01 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 16 Aug 2016 23:42:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,529,1464678000"; d="scan'208";a="866761785" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga003.jf.intel.com with ESMTP; 16 Aug 2016 23:42:00 -0700 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 16 Aug 2016 23:41:59 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 16 Aug 2016 23:41:59 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.181]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.107]) with mapi id 14.03.0248.002; Wed, 17 Aug 2016 14:41:57 +0800 From: "Wang, Zhihong" To: Yuanhan Liu CC: Maxime Coquelin , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] optimize vhost enqueue Thread-Index: AQHR960YCQUsXmkhfkCfaLs++FbB76BLF3QAgAFB2ND//5IYgIAAxTew Date: Wed, 17 Aug 2016 06:41:57 +0000 Message-ID: <8F6C2BD409508844A0EFC19955BE09411077220A@SHSMSX103.ccr.corp.intel.com> References: <1471319402-112998-1-git-send-email-zhihong.wang@intel.com> <8F6C2BD409508844A0EFC19955BE09411077206B@SHSMSX103.ccr.corp.intel.com> <20160817023825.GO30752@yliu-dev.sh.intel.com> In-Reply-To: <20160817023825.GO30752@yliu-dev.sh.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGQ1OWU3N2EtNzAwYy00NWMxLWE1ODUtZDY5MzI3YTY4YjFlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InFlSXZFODUyUlQyR2Q0OHJ1R3dxTnJuTGpBMWJnT1FzTUd0c0FuQ0o4ME09In0= x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue 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, 17 Aug 2016 06:42:02 -0000 > -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > Sent: Wednesday, August 17, 2016 10:38 AM > To: Wang, Zhihong > Cc: Maxime Coquelin ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue >=20 > On Wed, Aug 17, 2016 at 01:45:26AM +0000, Wang, Zhihong wrote: > > > > > > > -----Original Message----- > > > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] > > > Sent: Tuesday, August 16, 2016 10:00 PM > > > To: Wang, Zhihong ; dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue > > > > > > Hi Zhihong, > > > > > > On 08/16/2016 05:50 AM, Zhihong Wang wrote: > > > > This patch optimizes the vhost enqueue function: > rte_vhost_enqueue_burst. > > > > > > > > Currently there're 2 callbacks for vhost enqueue: > > > > * virtio_dev_merge_rx for mrg_rxbuf turned on cases. > > > > * virtio_dev_rx for mrg_rxbuf turned off cases. > > > > > > > > The virtio_dev_merge_rx doesn't provide optimal performance, also i= t is > > > > reported having compatibility issue working with Windows VMs. > > > Could you tell us more please about this compatibility issue? > > > > > > For example, when you have testpmd in the host and Window VM as the > guest, > > with mrg_rxbuf turned on, the guest will hang once there's packets enqu= eued > > by virtio_dev_merge_rx. >=20 > You should put it into commit log. Okay. >=20 > > Let me know if you see the same issue. > > > > > > > > > > > > > > > Besides, having 2 separated functions increases maintenance efforts= . > > > > > > > > This patch uses a single function logic to replace the current 2 fo= r > > > > better maintainability, and provides better performance by optimizi= ng > > > > caching behavior especially for mrg_rxbuf turned on cases. >=20 > Here, here sounds two parts to me: >=20 > - one to unite mergeable and non-mergeable Rx >=20 > - another one to optimize the mergeable path >=20 > That means you should do it in two patches, with that we can have clear > understanding what changes the performance boost. It also helps review. Please see explanation below. >=20 > > > Do you have some benchmark comparison before and after your change? > > > > > > Also, for maintainability, I would suggest the that the enqueue > > > function be split. Because vhost_enqueue_burst becomes very long (220 > > > LoC), and max level of indentation is too high (6). > > > > > > It makes the code hard to understand, and prone to miss bugs during > > > review and maintenance. >=20 > Agreed. >=20 > > > > This is something I've thought about while writing the code, the reason= I > > keep it as one function body is that: > > > > 1. This function is very performance sensitive, and we need full contr= ol of > > code ordering (You can compare with the current performance with th= e > > mrg_rxbuf feature turned on to see the difference). >=20 > Will inline functions help? Optimization in this patch actually reorganizes the code from its logic, so it's not suitable for making separated functions. I'll explain this in v2. >=20 > > 2. I somehow find that a single function logic makes it easier to unde= rstand, > > surely I can add comments to make it easiler to read for . > > > > Please let me know if you still insist, we can discuss more on it. >=20 > I am personally not a fan of huge function; I would try hard to avoid > too many levels of indentation as well. >=20 > > > > > > > > > > > > > It also fixes the issue working with Windows VMs. > > > Ideally, the fix should be sent separately, before the rework. > > > Indeed, we might want to have the fix in the stable branch, without > > > picking the optimization. >=20 > Agreed. The fact is that I don't have much time to debug with the current code since it's messy and I don't have Windows virtio code and the debugging environment. This patch doesn't try to fix this issue, it rewrites the logic totally, and somehow fixes this issue. Do you think integrating this whole patch into the stable branch will work? Personally I think it makes more sense. >=20 > > > > > > > > > > > Signed-off-by: Zhihong Wang > > > > --- > > > > lib/librte_vhost/vhost-net.h | 6 +- > > > > lib/librte_vhost/vhost_rxtx.c | 582 > ++++++++++++++---------------------------- > > > > lib/librte_vhost/virtio-net.c | 15 +- > > > > 3 files changed, 208 insertions(+), 395 deletions(-) > > > 582 lines changed is a huge patch. > > > If possible, it would be better splitting it in incremental changes, > > > making the review process easier. > > > > > > It looks like a huge patch, but it simply deletes the current implement= ation > > and add the new code. I think perhaps split it into 2, 1st one to repla= ce > > just the rte_vhost_enqueue_burst, 2nd one to delete all the obsolete > functions. > > It should make the patch clear, how do you think? :) >=20 > Nope, it's not working in that way. It should be: >=20 > - one patch to fix the hang issue for windows guest >=20 > Please cc it to stable@dpdk.org as well so that we could pick it for > v16.07 stable release. >=20 > - one patch to unite the two different Rx code path >=20 > - another patch to optimize mergeable code path I can separate optimization from the basic code in v2, however as I explain= ed this patch is built from scratch and doesn't take anything from the existin= g code, so there's no way to transform from the existing code incrementally i= nto the new code. >=20 > --yliu