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