From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id EFF4A2C66 for ; Wed, 17 Aug 2016 03:45:31 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP; 16 Aug 2016 18:45:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,529,1464678000"; d="scan'208";a="866658791" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga003.jf.intel.com with ESMTP; 16 Aug 2016 18:45:30 -0700 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 16 Aug 2016 18:45:30 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 16 Aug 2016 18:45:29 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.181]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.150]) with mapi id 14.03.0248.002; Wed, 17 Aug 2016 09:45:27 +0800 From: "Wang, Zhihong" To: Maxime Coquelin , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] optimize vhost enqueue Thread-Index: AQHR960YCQUsXmkhfkCfaLs++FbB76BLF3QAgAFB2NA= Date: Wed, 17 Aug 2016 01:45:26 +0000 Message-ID: <8F6C2BD409508844A0EFC19955BE09411077206B@SHSMSX103.ccr.corp.intel.com> References: <1471319402-112998-1-git-send-email-zhihong.wang@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTNiNDY3ZGYtNDI4Yy00NTNkLWE3ZmUtMWFkN2U1MmI3YmU1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlZkMnNITnk5OTR1QVVZNjhUWFIxZ1RzNUZkSmFvR015dkFWcHZQNFwvYzlvPSJ9 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 01:45:32 -0000 > -----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 >=20 > Hi Zhihong, >=20 > On 08/16/2016 05:50 AM, Zhihong Wang wrote: > > This patch optimizes the vhost enqueue function: rte_vhost_enqueue_burs= t. > > > > 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 it 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 enqueued by virtio_dev_merge_rx. Let me know if you see the same issue. >=20 > > > > Besides, having 2 separated functions increases maintenance efforts. > > > > This patch uses a single function logic to replace the current 2 for > > better maintainability, and provides better performance by optimizing > > caching behavior especially for mrg_rxbuf turned on cases. > Do you have some benchmark comparison before and after your change? >=20 > 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). >=20 > It makes the code hard to understand, and prone to miss bugs during > review and maintenance. 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 control o= f code ordering (You can compare with the current performance with the mrg_rxbuf feature turned on to see the difference). 2. I somehow find that a single function logic makes it easier to understa= nd, 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 > > > > 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 > > > > 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 implementatio= n and add the new code. I think perhaps split it into 2, 1st one to replace just the rte_vhost_enqueue_burst, 2nd one to delete all the obsolete functi= ons. It should make the patch clear, how do you think? :) >=20 > Also, for v2, please prefix the commit title with "vhost:". Thanks for the hint! Will do. >=20 > Thanks for your contribution, I'm looking forward for the v2. > - Maxime