From: "Wang, Zhihong" <zhihong.wang@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue
Date: Wed, 17 Aug 2016 10:07:00 +0000 [thread overview]
Message-ID: <8F6C2BD409508844A0EFC19955BE094110772338@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <020de331-94f0-049a-6e7d-30825faf54dd@redhat.com>
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, August 17, 2016 5:18 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue
>
>
>
> 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 <zhihong.wang@intel.com>
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; 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 <zhihong.wang@intel.com>; 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 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.
> >>
> >> 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 for
> >>>>> better maintainability, and provides better performance by optimizing
> >>>>> 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 clear
> >> 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 (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.
> >>
> >> Agreed.
> >>
> >>>
> >>> 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 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.
>
> I agree with Yuanhan.
> Inline functions should not break the optimizations.
> IMHO, this is mandatory for the patch to be accepted.
Excellent!
>
> >
> >
> >>
> >>> 2. I somehow find that a single function logic makes it easier to understand,
> >>> 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.
>
> It seems you are not the only one facing the issue:
> https://github.com/YanVugenfirer/kvm-guest-drivers-windows/issues/70
>
> 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.
>
> > 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.
>
> 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.
>
> And the stable branch is not meant for integrating such big reworks,
> it is only meant to fix bugs.
>
> The risk of regressions have to be avoided as much as possible.
>
> >
> >
> >>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
> >>>>> ---
> >>>>> 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 implementation
> >>> 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
> >> 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 explained
> > this patch is built from scratch and doesn't take anything from the existing
> > code, so there's no way to transform from the existing code incrementally into
> > the new code.
> >
> >
> >>
> >> --yliu
next prev parent reply other threads:[~2016-08-17 10:07 UTC|newest]
Thread overview: 140+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-16 3:50 Zhihong Wang
2016-08-16 13:59 ` Maxime Coquelin
2016-08-17 1:45 ` Wang, Zhihong
2016-08-17 2:38 ` Yuanhan Liu
2016-08-17 6:41 ` Wang, Zhihong
2016-08-17 9:17 ` Maxime Coquelin
2016-08-17 9:51 ` Yuanhan Liu
2016-08-18 13:44 ` Wang, Zhihong
2016-08-17 10:07 ` Wang, Zhihong [this message]
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 0/6] vhost: optimize enqueue Zhihong Wang
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 1/6] vhost: rewrite enqueue Zhihong Wang
2016-08-19 2:39 ` Yuanhan Liu
2016-08-19 7:07 ` Wang, Zhihong
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 2/6] vhost: remove obsolete Zhihong Wang
2016-08-19 2:32 ` Yuanhan Liu
2016-08-19 7:08 ` Wang, Zhihong
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 3/6] vhost: remove useless volatile Zhihong Wang
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 4/6] vhost: add desc prefetch Zhihong Wang
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 5/6] vhost: batch update used ring Zhihong Wang
2016-08-18 6:33 ` [dpdk-dev] [PATCH v2 6/6] vhost: optimize cache access Zhihong Wang
2016-08-19 5:43 ` [dpdk-dev] [PATCH v3 0/5] vhost: optimize enqueue Zhihong Wang
2016-08-19 5:43 ` [dpdk-dev] [PATCH v3 1/5] vhost: rewrite enqueue Zhihong Wang
2016-08-22 9:35 ` Maxime Coquelin
2016-08-23 2:27 ` Wang, Zhihong
2016-08-25 4:00 ` Yuanhan Liu
2016-08-19 5:43 ` [dpdk-dev] [PATCH v3 2/5] vhost: remove useless volatile Zhihong Wang
2016-08-19 5:43 ` [dpdk-dev] [PATCH v3 3/5] vhost: add desc prefetch Zhihong Wang
2016-08-19 5:43 ` [dpdk-dev] [PATCH v3 4/5] vhost: batch update used ring Zhihong Wang
2016-08-25 3:48 ` Yuanhan Liu
2016-08-25 5:19 ` Wang, Zhihong
2016-08-19 5:43 ` [dpdk-dev] [PATCH v3 5/5] vhost: optimize cache access Zhihong Wang
2016-08-22 8:11 ` [dpdk-dev] [PATCH v3 0/5] vhost: optimize enqueue Maxime Coquelin
2016-08-22 10:01 ` Maxime Coquelin
2016-08-22 10:35 ` Thomas Monjalon
2016-08-24 3:37 ` Wang, Zhihong
2016-08-23 2:31 ` Wang, Zhihong
2016-08-23 10:43 ` Wang, Zhihong
2016-08-23 12:16 ` Maxime Coquelin
2016-08-23 12:22 ` Yuanhan Liu
2016-08-23 2:15 ` Wang, Zhihong
2016-09-21 8:50 ` Jianbo Liu
2016-09-21 9:27 ` Wang, Zhihong
2016-09-21 12:54 ` Jianbo Liu
2016-09-22 2:11 ` Wang, Zhihong
2016-09-22 2:29 ` Yuanhan Liu
2016-09-22 5:47 ` Jianbo Liu
2016-09-22 6:58 ` Wang, Zhihong
2016-09-22 9:01 ` Jianbo Liu
2016-09-22 10:04 ` Wang, Zhihong
2016-09-22 14:41 ` Jianbo Liu
2016-09-23 2:56 ` Wang, Zhihong
2016-09-23 10:41 ` Jianbo Liu
2016-09-23 13:41 ` Thomas Monjalon
2016-09-25 5:41 ` Wang, Zhihong
2016-09-26 5:12 ` Jianbo Liu
2016-09-26 5:25 ` Wang, Zhihong
2016-09-26 5:38 ` Jianbo Liu
2016-09-26 6:00 ` Wang, Zhihong
2016-09-26 4:24 ` Jianbo Liu
2016-09-26 5:37 ` Luke Gorrie
2016-09-26 5:40 ` Jianbo Liu
2016-09-27 10:21 ` Yuanhan Liu
2016-09-27 16:45 ` Wang, Zhihong
2016-10-09 12:09 ` Wang, Zhihong
2016-10-10 2:44 ` Yuanhan Liu
2016-10-10 5:31 ` Jianbo Liu
2016-10-10 6:22 ` Wang, Zhihong
2016-10-10 6:57 ` Jianbo Liu
2016-10-10 7:25 ` Wang, Zhihong
2016-10-12 2:53 ` Yuanhan Liu
2016-10-12 12:22 ` Wang, Zhihong
2016-10-12 15:31 ` Thomas Monjalon
2016-10-13 1:21 ` Wang, Zhihong
2016-10-13 3:51 ` Jianbo Liu
2016-10-13 5:33 ` Yuanhan Liu
2016-10-13 5:35 ` Yuanhan Liu
2016-10-13 6:02 ` Wang, Zhihong
2016-10-13 7:54 ` Maxime Coquelin
2016-10-13 9:23 ` Maxime Coquelin
2016-10-14 10:11 ` Yuanhan Liu
2016-08-30 3:35 ` [dpdk-dev] [PATCH v4 0/6] " Zhihong Wang
2016-08-30 3:35 ` [dpdk-dev] [PATCH v4 1/6] vhost: fix windows vm hang Zhihong Wang
2016-09-05 5:24 ` [dpdk-dev] [dpdk-stable] " Yuanhan Liu
2016-09-05 5:25 ` Wang, Zhihong
2016-09-05 5:40 ` Yuanhan Liu
2016-08-30 3:36 ` [dpdk-dev] [PATCH v4 2/6] vhost: rewrite enqueue Zhihong Wang
2016-09-05 6:39 ` Yuanhan Liu
2016-09-07 5:33 ` Yuanhan Liu
2016-09-07 5:39 ` Wang, Zhihong
2016-08-30 3:36 ` [dpdk-dev] [PATCH v4 3/6] vhost: remove useless volatile Zhihong Wang
2016-08-30 3:36 ` [dpdk-dev] [PATCH v4 4/6] vhost: add desc prefetch Zhihong Wang
2016-08-30 3:36 ` [dpdk-dev] [PATCH v4 5/6] vhost: batch update used ring Zhihong Wang
2016-08-30 3:36 ` [dpdk-dev] [PATCH v4 6/6] vhost: optimize cache access Zhihong Wang
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 0/6] vhost: optimize enqueue Zhihong Wang
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 1/6] vhost: fix windows vm hang Zhihong Wang
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 2/6] vhost: rewrite enqueue Zhihong Wang
2016-09-12 15:42 ` Maxime Coquelin
2016-09-14 8:20 ` Wang, Zhihong
2016-09-15 16:35 ` Maxime Coquelin
2016-09-12 16:26 ` Maxime Coquelin
2016-09-14 8:22 ` Wang, Zhihong
2016-09-18 14:19 ` Yuanhan Liu
2016-09-19 3:29 ` Wang, Zhihong
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 3/6] vhost: remove useless volatile Zhihong Wang
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 4/6] vhost: add desc prefetch Zhihong Wang
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 5/6] vhost: batch update used ring Zhihong Wang
2016-09-12 15:45 ` Maxime Coquelin
2016-09-14 8:43 ` Wang, Zhihong
2016-09-15 16:38 ` Maxime Coquelin
2016-09-18 2:55 ` Yuanhan Liu
2016-09-18 2:57 ` Wang, Zhihong
2016-09-09 3:39 ` [dpdk-dev] [PATCH v5 6/6] vhost: optimize cache access Zhihong Wang
2016-09-12 13:52 ` [dpdk-dev] [PATCH v5 0/6] vhost: optimize enqueue Maxime Coquelin
2016-09-12 13:56 ` Maxime Coquelin
2016-09-12 14:01 ` Yuanhan Liu
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 " Zhihong Wang
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 1/6] vhost: fix windows vm hang Zhihong Wang
2016-10-13 6:18 ` [dpdk-dev] [dpdk-stable] " Yuanhan Liu
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 2/6] vhost: rewrite enqueue Zhihong Wang
2016-09-22 9:58 ` Jianbo Liu
2016-09-22 10:13 ` Wang, Zhihong
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 3/6] vhost: remove useless volatile Zhihong Wang
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 4/6] vhost: add desc prefetch Zhihong Wang
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 5/6] vhost: batch update used ring Zhihong Wang
2016-09-20 2:00 ` [dpdk-dev] [PATCH v6 6/6] vhost: optimize cache access Zhihong Wang
2016-09-21 2:26 ` [dpdk-dev] [PATCH v6 0/6] vhost: optimize enqueue Yuanhan Liu
2016-09-21 4:39 ` Maxime Coquelin
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 0/7] vhost: optimize mergeable Rx path Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 1/7] vhost: remove useless volatile Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 2/7] vhost: optimize cache access Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 3/7] vhost: simplify mergeable Rx vring reservation Yuanhan Liu
2016-10-25 22:08 ` Thomas Monjalon
2016-10-26 2:56 ` Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 4/7] vhost: use last avail idx for avail ring reservation Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 5/7] vhost: shadow used ring update Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 6/7] vhost: prefetch avail ring Yuanhan Liu
2016-10-14 9:34 ` [dpdk-dev] [PATCH v7 7/7] vhost: retrieve avail head once Yuanhan Liu
2016-10-18 2:25 ` [dpdk-dev] [PATCH v7 0/7] vhost: optimize mergeable Rx path Jianbo Liu
2016-10-18 14:53 ` Maxime Coquelin
2016-10-21 7:51 ` Yuanhan Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8F6C2BD409508844A0EFC19955BE094110772338@SHSMSX103.ccr.corp.intel.com \
--to=zhihong.wang@intel.com \
--cc=dev@dpdk.org \
--cc=maxime.coquelin@redhat.com \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).