[dpdk-dev] [PATCH] optimize vhost enqueue

[Yuanhan Liu]

On Wed, Aug 17, 2016 at 01:45:26AM +0000, Wang, Zhihong wrote:
> 
> 
> > -----Original Message-----
> > From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> > Sent: Tuesday, August 16, 2016 10:00 PM
> > To: Wang, Zhihong <zhihong.wang at intel.com>; dev at 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.

> 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.

> > 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?

>  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.

> > 
> > >
> > > Signed-off-by: Zhihong Wang <zhihong.wang at 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 at 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

	--yliu