From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Zhihong" Subject: Re: [PATCH] optimize vhost enqueue Date: Wed, 17 Aug 2016 10:07:00 +0000 Message-ID: <8F6C2BD409508844A0EFC19955BE094110772338@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> <8F6C2BD409508844A0EFC19955BE09411077220A@SHSMSX103.ccr.corp.intel.com> <020de331-94f0-049a-6e7d-30825faf54dd@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Maxime Coquelin , Yuanhan Liu Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id B97CB56AB for ; Wed, 17 Aug 2016 12:07:05 +0200 (CEST) In-Reply-To: <020de331-94f0-049a-6e7d-30825faf54dd@redhat.com> 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: Wednesday, August 17, 2016 5:18 PM > To: Wang, Zhihong ; Yuanhan Liu > > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue >=20 >=20 >=20 > On 08/17/2016 08:41 AM, Wang, Zhihong wrote: > > > > > >> -----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 > >> > >> 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 > enqueued > >>> by virtio_dev_merge_rx. > >> > >> You should put it into commit log. > > > > > > Okay. > > > > > >> > >>> 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. > >> > >> Here, here sounds two parts to me: > >> > >> - one to unite mergeable and non-mergeable Rx > >> > >> - another one to optimize the mergeable path > >> > >> That means you should do it in two patches, with that we can have clea= r > >> understanding what changes the performance boost. It also helps review= . > > > > > > Please see explanation below. > > > > > >> > >>>> 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 (22= 0 > >>>> 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. > >> > >> Agreed. > >> > >>> > >>> This is something I've thought about while writing the code, the reas= on I > >>> keep it as one function body is that: > >>> > >>> 1. This function is very performance sensitive, and we need full con= trol of > >>> code ordering (You can compare with the current performance with = the > >>> mrg_rxbuf feature turned on to see the difference). > >> > >> 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 > I agree with Yuanhan. > Inline functions should not break the optimizations. > IMHO, this is mandatory for the patch to be accepted. Excellent! >=20 > > > > > >> > >>> 2. I somehow find that a single function logic makes it easier to un= derstand, > >>> 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. > >> > >> I am personally not a fan of huge function; I would try hard to avoid > >> too many levels of indentation as well. > >> > >>> > >>>> > >>>>> > >>>>> 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. > >> > >> 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. >=20 > It seems you are not the only one facing the issue: > https://github.com/YanVugenfirer/kvm-guest-drivers-windows/issues/70 >=20 > So a dedicated fix is really important. Yeah that's me raising this issue there. But I think it's another standalone task to identify the root cause and find the fix for the existing code. >=20 > > 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 w= ork? > > Personally I think it makes more sense. >=20 > No. > We don't even know why/how it fixes the Windows issue, which would be > the first thing to understand before integrating a fix in stable branch. >=20 > And the stable branch is not meant for integrating such big reworks, > it is only meant to fix bugs. >=20 > The risk of regressions have to be avoided as much as possible. >=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 impleme= ntation > >>> and add the new code. I think perhaps split it into 2, 1st one to rep= lace > >>> just the rte_vhost_enqueue_burst, 2nd one to delete all the obsolete > >> functions. > >>> It should make the patch clear, how do you think? :) > >> > >> Nope, it's not working in that way. It should be: > >> > >> - one patch to fix the hang issue for windows guest > >> > >> Please cc it to stable@dpdk.org as well so that we could pick it for > >> v16.07 stable release. > >> > >> - one patch to unite the two different Rx code path > >> > >> - another patch to optimize mergeable code path > > > > > > I can separate optimization from the basic code in v2, however as I exp= lained > > this patch is built from scratch and doesn't take anything from the exi= sting > > code, so there's no way to transform from the existing code incremental= ly into > > the new code. > > > > > >> > >> --yliu