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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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.