Re: [PATCH] optimize vhost enqueue

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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?

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

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

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

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