[dpdk-dev] [PATCH] optimize vhost enqueue

[Wang, Zhihong]

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

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


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

 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.


> 
> >
> > 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.
> 
> >
> > 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?  :)


> 
> Also, for v2, please prefix the commit title with "vhost:".

Thanks for the hint! Will do.

> 
> Thanks for your contribution, I'm looking forward for the v2.
> - Maxime