From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id EC4DA3781 for ; Wed, 20 May 2015 00:41:20 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 19 May 2015 15:41:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,460,1427785200"; d="scan'208";a="573921623" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga003.jf.intel.com with ESMTP; 19 May 2015 15:41:18 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.59]) by IRSMSX109.ger.corp.intel.com ([169.254.13.51]) with mapi id 14.03.0224.002; Tue, 19 May 2015 23:41:18 +0100 From: "Dumitrescu, Cristian" To: Thomas Monjalon , "Wodkowski, PawelX" Thread-Topic: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pipeline ports and tables Thread-Index: AQHQkWZQUPiGW1BlN0muPEnhvuILX52D1y8Q Date: Tue, 19 May 2015 22:41:17 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891263236A122@IRSMSX108.ger.corp.intel.com> References: <1430396143-10936-1-git-send-email-michalx.k.jastrzebski@intel.com> <3EB4FA525960D640B5BDFFD6A3D891263235BBA3@IRSMSX108.ger.corp.intel.com> <5383811.jFa36Ozyeg@xps13> In-Reply-To: <5383811.jFa36Ozyeg@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pipeline ports and tables 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, 19 May 2015 22:41:21 -0000 Hi Thomas, I am asking for your help to identify a technical solution for moving forwa= rd. Enabling the stats is a very important part of our Packet Framework enh= ancements, as it is a dependency for a set of patches we plan to submit nex= t. 1. What is the technical solution to avoid performance loss due to stats su= pport? Generally, I would agree with you that config options should be avoided, es= pecially those that alter the API (function prototypes, data structure layo= uts, etc). Please note this is not the case for any of our patches, as only= the stats collection is enabled/disabled, while the data structures and fu= nctions are not changed by the build time option. But what can you do for statistics? We all know that collecting the stats s= ucks up CPU cycles, especially due to memory accesses, so stats always have= a run-time cost. Traditionally, stats are typically enabled for debugging = purposes and then turned off for the release version when performance is re= quired. How can this be done if build time flags are not used? Enabling stats conditionally at run-time has a significant cost potentially= , as this involves adding branches to the code; this is particularly bad du= e to branches being added in the library code, which is not aware of the ov= erhead of the application running on top of it on the same CPU core. We are= battling cycles very hard in the Packet Framework, trying to avoid any was= te, and the cost of branch misprediction of ~15 cycles is prohibitive when = your packet budget (for the overall app, not just the library) is less than= ~200 cycles. Sometimes, branches are predicted correctly by the CPU, some = other times not, for example when the application code adds a lot of additi= onal branches (which is the typical case for an app). This is outside of th= e control of the library. I would not recommend the run-time conditional option for stats support in = DPDK, where performance is key. The only option that looks feasible to me t= o avoid performance impact is well-behaved build time option (i.e. those th= at does not change the API functions and data structures). 2. You already agreed that having build time options for performance reaso= ns is fine, why change now? We had a previous email thread on QoS stats (http://www.dpdk.org/ml/archive= s/dev/2015-February/013820.html ), and you stated: >We must remove and avoid build-time options. >The only ones which might be acceptable are the ones which allow more >performance by disabling some features. I think stats support is the perfect example where this is applicable. We t= ook this as the guidance from you, and we worked out these stats exactly as= you recommended. Let me also mention that the format of the API functions = and data structures for stats were also designed to match exactly the requi= rements from this discussion (e.g. clear on read approach, etc). Our functi= ons match exactly the strategy that Stephen and myself agreed on at that ti= me. Our work was done under this agreement. 3. There are other libraries that already apply the build time option for s= tats support Other DPDK libraries provide stats based on build time configuration: Ethde= v, QoS scheduler, IP fragmentation. Are you suggesting we need to remove th= e build time stats option for these libraries, too? If yes, what about the = performance impact, which would be prohibitive (I can definitely testify th= is for QOS scheduler, where every cycle counts). This would result in choos= ing between functionality (stats support) vs. performance, and it might be = that for some of these libraries the decision will be to remove the stats s= upport altogether in order to keep the performance unchanged. I think we should encourage people to add stats to more libraries, and if w= e forbid build-time option for stats support we send a strong signal to dis= courage people from adding stats support to the applicable DPDK libraries. 4. Coding guidelines Is there a rule on "build time options are no longer allowed in DPDK" publi= shed somewhere in our documentation? Is it part of the coding standard? Am = I wrong to say this is not the case at this point? If we intend to have such a rule, then I think it needs to be discussed on = this list and published, to avoid any misinterpretation (leading to rework= and debates). It should be published together with all the acceptable exce= ptions that are part of any rule definition. I personally think the stats s= upport should be such an exception. Thanks for your help! Regards, Cristian > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Monday, May 18, 2015 12:02 PM > To: Wodkowski, PawelX > Cc: dev@dpdk.org; Dumitrescu, Cristian; Jastrzebski, MichalX K > Subject: Re: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pi= peline > ports and tables >=20 > 2015-05-05 15:11, Dumitrescu, Cristian: > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Jastrzebski > > > From: Pawel Wodkowski > > > > > > This patch adds statistics collection for librte_pipeline. > > > Those statistics ale disabled by default during build time. > > > > > > Signed-off-by: Pawel Wodkowski > > > --- > > > config/common_bsdapp | 1 + > > > config/common_linuxapp | 1 + > [...] > > > # Compile librte_pipeline > > > # > > > CONFIG_RTE_LIBRTE_PIPELINE=3Dy > > > +CONFIG_RTE_PIPELINE_STATS_COLLECT=3Dn > [...] > > > > Acked by: Cristian Dumitrescu >=20 > Nack because of new config option. > The same problem appear for all series related to packet framework.