From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 7C67C58CB for ; Wed, 20 May 2015 02:07:02 +0200 (CEST) Received: by wgjc11 with SMTP id c11so35311300wgj.0 for ; Tue, 19 May 2015 17:07:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=YzSQzdizNLTEtFBW7R9a1qJtugl6KjXIAjN3vHd2Gok=; b=RW1myrYPf78Nm8gtqudA14rFn3Qd75PkTbQx/ZcQkSEYsX3QsWcfUqCKelM8Ot40mV AWnpNkCMoABddv1jsp2erPs749+1Nx6HwvqL75OWxAARY4p6yfmA8VHenaUS7jTEFT0q eDXK54Q2tq9K1SsaAkybVMbbSANyFW8dm8/IQ5eR8Kxe11x7wCf/vtuHmQphMhlDAmgE rVggfHgbpc+9Q9udbPIIwXVKeF5MOALOhbIonqvjNF9uo77lCD7lS8p/yJC03NS3d9H2 vKwD3XJWLSY7vMrJcUBfkOEIzlwTFz0j9G3UbLMQUVRtEdeHIm9w4fHWTpLK1AivBuVc D3WQ== X-Gm-Message-State: ALoCoQnhmZwnImxCfu1sRaG+/T+pfGMg/t4PD3uSG2mL4JU8JjRKt+G7VdQmIEj96dk5oRyESPlU X-Received: by 10.180.218.108 with SMTP id pf12mr37081215wic.13.1432080422331; Tue, 19 May 2015 17:07:02 -0700 (PDT) Received: from xps13.localnet (2E6B6683.dsl.pool.telekom.hu. [46.107.102.131]) by mx.google.com with ESMTPSA id ew10sm623378wic.22.2015.05.19.17.07.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 May 2015 17:07:01 -0700 (PDT) From: Thomas Monjalon To: "Dumitrescu, Cristian" Date: Wed, 20 May 2015 02:06:16 +0200 Message-ID: <43396928.Zqf8pLaHBD@xps13> Organization: 6WIND User-Agent: KMail/4.14.7 (Linux/4.0.1-1-ARCH; KDE/4.14.7; x86_64; ; ) In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891263236A122@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> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 00:07:02 -0000 Hi Cristian, Thanks for the detailed explanation. 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 It's good to have this discussion and let other contributors giving their opinion. 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 stats > support? > Generally, I would agree with you that config options should be avoided, > especially those that alter the API (function prototypes, data structure > 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 stats > 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 not > used? 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) > Enabling stats conditionally at run-time has a significant cost > potentially, as this involves adding branches to the code; this is Yes, a run-time option (similar to run-time log level) will cost only an "unlikely if" branching. (third solution) > 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. This kind of issue applies to many areas of DPDK. > Sometimes, branches are predicted correctly by the CPU, some other > times not, for example when the application code adds a lot of additional > 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). The compile-time option makes testing coverage really complex. (fourth solution) > 2. You already agreed that having build time options for performance > reasons is fine, why change now? Right, build time options are tolerated in DPDK because it is a performance 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" > 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 approach, > 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 for > stats support That's why I suggested to remove this kind of option in the other libraries. > Other DPDK libraries provide stats based on build time configuration: > Ethdev, QoS scheduler, IP fragmentation. ethdev? Are you referring to RTE_ETHDEV_QUEUE_STAT_CNTRS which is a max? > Are you suggesting we need to remove the build time stats option for > these libraries, too? Yes for CONFIG_RTE_LIBRTE_IP_FRAG_TBL_STAT and CONFIG_RTE_SCHED_COLLECT_STATS, but it's open to comments from others. > If yes, what about the performance impact, which would be prohibitive > (I can definitely testify this for QOS scheduler, where every cycle counts). > This would result in choosing between functionality (stats support) vs. > performance, and it might be that for some of these libraries the decision > will be to remove the stats support altogether in order to keep the > performance unchanged. 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). > 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 signal > to discourage people from adding stats support to the applicable DPDK > libraries. That's a simple fifth solution: removing statistics from these libraries. But I feel it's a bad idea. > 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? No, no and no you are not wrrong;) > 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. You are totally right. We must start writing this kind of document. > I personally think the stats support should be such an exception. That's your opinion and we have to carefully read opinion from others. > Thanks for your help! Thanks for the contribution, 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_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=y > > > > +CONFIG_RTE_PIPELINE_STATS_COLLECT=n > > [...] > > > > > > Acked by: Cristian Dumitrescu > > > > Nack because of new config option. > > The same problem appear for all series related to packet framework.