Re: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pipeline ports and tables

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

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_pipeline
> ports and tables
> 
> 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 <pawelx.wdkowski@intel.com>
> > > > >
> > > > > This patch adds statistics collection for librte_pipeline.
> > > > > Those statistics ale disabled by default during build time.
> > > > >
> > > > > Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> > > > > ---
> > > > >  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 <cristian.dumitrescu@intel.com>
> > >
> > > Nack because of new config option.
> > > The same problem appear for all series related to packet framework.
>