From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8DD9EC04D280; Tue, 16 Aug 2016 13:59:54 +0000 (UTC) Received: from [10.36.4.37] (vpn1-4-37.ams2.redhat.com [10.36.4.37]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7GDxqR4004449 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 16 Aug 2016 09:59:53 -0400 From: Maxime Coquelin To: Zhihong Wang , dev@dpdk.org References: <1471319402-112998-1-git-send-email-zhihong.wang@intel.com> Message-ID: Date: Tue, 16 Aug 2016 15:59:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1471319402-112998-1-git-send-email-zhihong.wang@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 16 Aug 2016 13:59:54 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] optimize vhost enqueue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Aug 2016 13:59:55 -0000 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