From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Zhihong" Subject: Re: [PATCH] optimize vhost enqueue 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: Maxime Coquelin , "dev@dpdk.org" 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) In-Reply-To: Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----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