From mboxrd@z Thu Jan 1 00:00:00 1970 From: Panu Matilainen Subject: Re: [PATCH] vhost: add pmd xstats Date: Wed, 24 Aug 2016 15:37:10 +0300 Message-ID: References: <1471608966-39077-1-git-send-email-zhiyong.yang@intel.com> <45bf611c-fbd5-d485-566a-5752cd58ed7c@redhat.com> <20160824054602.GS30752@yliu-dev.sh.intel.com> <3180838.i4arGqJgvX@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, "Yang, Zhiyong" To: Thomas Monjalon , Yuanhan Liu Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id B2F82FFA for ; Wed, 24 Aug 2016 14:37:12 +0200 (CEST) In-Reply-To: <3180838.i4arGqJgvX@xps13> 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" On 08/24/2016 11:44 AM, Thomas Monjalon wrote: > 2016-08-24 13:46, Yuanhan Liu: >> On Tue, Aug 23, 2016 at 12:45:54PM +0300, Panu Matilainen wrote: >>>>>> Since collecting data of vhost_update_packet_xstats will have some >>>>>> effect on RX/TX performance, so, Setting compiling switch >>>>>> CONFIG_RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS=n by default in the >>>>> file >>>>>> config/common_base, if needing xstats data, you can enable it(y). >>>>> >>>>> NAK, such things need to be switchable at run-time. >>>>> >>>>> - Panu - >>>> >>>> Considering the following reasons using the compiler switch, not >>>> command-line at run-time. >>>> >>>> 1.Similar xstats update functions are always collecting stats data in the >>>> background when rx/tx are running, such as the physical NIC or virtio, >>>> which have no switch. Compiler switch for vhost pmd xstats is added >>>> as a option when performance is viewed as critical factor. >>>> >>>> 2. No data structure and API in any layer support the xstats update switch >>>> at run-time. Common data structure (struct rte_eth_dev_data) has no >>>> device-specific data member, if implementing enable/disable of vhost_update >>>> _packet_xstats at run-time, must define a flag(device-specific) in it, >>>> because the definition of struct vhost_queue in the driver code >>>> (eth_vhost_rx/eth_vhost_tx processing)is not visible from device perspective. >>>> >>>> 3. I tested RX/TX with v1 patch (y) as reference based on Intel(R) >>>> Xeon(R) CPU E5-2699 v3 @ 2.30GHz, for 64byts packets in burst mode, 32 packets >>>> in one RX/TX processing. Overhead of vhost_update_packet_xstats is less than >>>> 3% for the rx/tx processing. It looks that vhost_update_packet_xstats has a >>>> limited effect on performance drop. >>> >>> Well, either the performance overhead is acceptable and it should always be >>> on (like with physical NICs I think). Or it is not. In which case it needs >>> to be turnable on and off, at run-time. Rebuilding is not an option in the >>> world of distros. >> >> I think the less than 3% overhead is acceptable here, that I agree with >> Panu we should always keep it on. If someone compains it later that even >> 3% is too big for them, let's consider to make it be switchable at >> run-time. Either we could introduce a generic eth API for that, Or >> just introduce a vhost one if that doesn't make too much sense to other >> eth drivers. > > +1 > It may have sense to introduce a generic run-time option for stats. > Yup, sounds good. - Panu -