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 B2F82FFA for ; Wed, 24 Aug 2016 14:37:12 +0200 (CEST) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 06C85C04B328; Wed, 24 Aug 2016 12:37:12 +0000 (UTC) Received: from sopuli.koti.laiskiainen.org (vpn1-7-27.ams2.redhat.com [10.36.7.27]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7OCbAm8027269; Wed, 24 Aug 2016 08:37:10 -0400 To: Thomas Monjalon , Yuanhan Liu 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> Cc: dev@dpdk.org, "Yang, Zhiyong" From: Panu Matilainen Message-ID: Date: Wed, 24 Aug 2016 15:37:10 +0300 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: <3180838.i4arGqJgvX@xps13> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 24 Aug 2016 12:37:12 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats 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: Wed, 24 Aug 2016 12:37:13 -0000 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 -