From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id DE8535A36 for ; Wed, 20 May 2015 15:57:14 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 20 May 2015 06:57:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,464,1427785200"; d="scan'208";a="732459250" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga002.jf.intel.com with ESMTP; 20 May 2015 06:57:12 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.59]) by IRSMSX151.ger.corp.intel.com ([169.254.4.102]) with mapi id 14.03.0224.002; Wed, 20 May 2015 14:57:12 +0100 From: "Dumitrescu, Cristian" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pipeline ports and tables Thread-Index: AQHQkWZQUPiGW1BlN0muPEnhvuILX52D/wVsgADlY3A= Date: Wed, 20 May 2015 13:57:11 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891263236A65D@IRSMSX108.ger.corp.intel.com> References: <1430396143-10936-1-git-send-email-michalx.k.jastrzebski@intel.com> <5383811.jFa36Ozyeg@xps13> <3EB4FA525960D640B5BDFFD6A3D891263236A122@IRSMSX108.ger.corp.intel.com> <43396928.Zqf8pLaHBD@xps13> In-Reply-To: <43396928.Zqf8pLaHBD@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] 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: Wed, 20 May 2015 13:57:15 -0000 Hi Thomas, Can you please describe what solution 2 on your list (build-time log level)= consists of? I see log level useful for printing messages when an event takes place, but= this is not what these stats patches are about. We want to poll for those = counters on demand: if the build-time flag is off, then the value of those = counters is 0; when the build-time is on, then the stats counters have the = real value. Basically, the build-time flag only enables/disables the update= of the counters at run-time, which is where CPU cycles are consumed. I am = not sure how the log levels can help here? Regards, Cristian > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Wednesday, May 20, 2015 1:06 AM > To: Dumitrescu, Cristian > Cc: Wodkowski, PawelX; dev@dpdk.org; Jastrzebski, MichalX K > Subject: Re: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pi= peline > ports and tables >=20 > Hi Cristian, >=20 > Thanks for the detailed explanation. >=20 > You are raising a trade-off problem about > feature/maintenance/performance. > I think we must choose among 4 solutions: > 1/ always enabled > 2/ build-time log level > 3/ run-time option > 4/ build-time option >=20 > It's good to have this discussion and let other contributors giving their > opinion. >=20 > 2015-05-19 22:41, Dumitrescu, Cristian: > > Hi Thomas, > > > > I am asking for your help to identify a technical solution for moving > > forward. Enabling the stats is a very important part of our Packet > > Framework enhancements, as it is a dependency for a set of patches > > we plan to submit next. > > > > 1. What is the technical solution to avoid performance loss due to stat= s > > support? > > Generally, I would agree with you that config options should be avoided= , > > especially those that alter the API (function prototypes, data structur= e > > layouts, 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 functions are not changed by the build time option. > > > > But what can you do for statistics? We all know that collecting the sta= ts > > sucks 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 required. How can this be done if build time flags are n= ot > > used? >=20 > Statistics in drivers are always enabled (first solution). > If those statistics are only used for debugging, why not using the build-= time > option CONFIG_RTE_LOG_LEVEL? (second solution) >=20 > > Enabling stats conditionally at run-time has a significant cost > > potentially, as this involves adding branches to the code; this is >=20 > Yes, a run-time option (similar to run-time log level) will cost only > an "unlikely if" branching. (third solution) >=20 > > particularly bad due to branches being added in the library code, > > which is not aware of the overhead 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 waste, 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. >=20 > This kind of issue applies to many areas of DPDK. >=20 > > Sometimes, branches are predicted correctly by the CPU, some other > > times not, for example when the application code adds a lot of addition= al > > branches (which is the typical case for an app). This is outside of the > > 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 to avoid performance impact is well-behaved build time option > > (i.e. those that does not change the API functions and data structures)= . >=20 > The compile-time option makes testing coverage really complex. > (fourth solution) >=20 > > 2. You already agreed that having build time options for performance > > reasons is fine, why change now? >=20 > Right, build time options are tolerated in DPDK because it is a performan= ce > oriented project. We also have to avoid 2 issues: > - increasing number of compile-time options hide some bugs > - binary distributions of DPDK cannot use these compile-time "features" >=20 > > We had a previous email thread on QoS stats > (http://www.dpdk.org/ml/archives/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 took 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 requirements from this discussion (e.g. clear on read appro= ach, > > etc). Our functions match exactly the strategy that Stephen and myself > > agreed on at that time. Our work was done under this agreement. > > > > > > 3. There are other libraries that already apply the build time option f= or > > stats support >=20 > That's why I suggested to remove this kind of option in the other librari= es. >=20 > > Other DPDK libraries provide stats based on build time configuration: > > Ethdev, QoS scheduler, IP fragmentation. >=20 > ethdev? Are you referring to RTE_ETHDEV_QUEUE_STAT_CNTRS which is a > max? >=20 > > Are you suggesting we need to remove the build time stats option for > > these libraries, too? >=20 > Yes for CONFIG_RTE_LIBRTE_IP_FRAG_TBL_STAT and > CONFIG_RTE_SCHED_COLLECT_STATS, > but it's open to comments from others. >=20 > > If yes, what about the performance impact, which would be prohibitive > > (I can definitely testify this for QOS scheduler, where every cycle cou= nts). > > This would result in choosing between functionality (stats support) vs. > > performance, and it might be that for some of these libraries the decis= ion > > will be to remove the stats support altogether in order to keep the > > performance unchanged. >=20 > Note that distributions must take this decision when packaging DPDK: > stats or extra performance? > The decision is easier if those options are clearly documented as debug > facilities. But in this case, using only 1 option (log level) would make > maintenance easier (counterpart is enabling all or none stats). >=20 > > I think we should encourage people to add stats to more libraries, and > > if we forbid build-time option for stats support we send a strong signa= l > > to discourage people from adding stats support to the applicable DPDK > > libraries. >=20 > That's a simple fifth solution: removing statistics from these libraries. > But I feel it's a bad idea. >=20 > > 4. Coding guidelines > > Is there a rule on "build time options are no longer allowed in DPDK" > > published somewhere in our documentation? Is it part of the coding > standard? > > Am I wrong to say this is not the case at this point? >=20 > No, no and no you are not wrrong;) >=20 > > 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 exceptions that are part of any rule definition. >=20 > You are totally right. We must start writing this kind of document. >=20 > > I personally think the stats support should be such an exception. >=20 > That's your opinion and we have to carefully read opinion from others. >=20 > > Thanks for your help! >=20 > Thanks for the contribution, Cristian. >=20 >=20 > > > -----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_pipeline > > > ports and tables > > > > > > 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 > > > > > > Nack because of new config option. > > > The same problem appear for all series related to packet framework. >=20