From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH] optimize vhost enqueue Date: Tue, 16 Aug 2016 15:59:52 +0200 Message-ID: References: <1471319402-112998-1-git-send-email-zhihong.wang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: Zhihong Wang , dev@dpdk.org Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 2BF6E6932 for ; Tue, 16 Aug 2016 15:59:55 +0200 (CEST) In-Reply-To: <1471319402-112998-1-git-send-email-zhihong.wang@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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