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

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

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?

Enabling stats conditionally at run-time has a significant cost potentially, as this involves adding branches to the code; this is 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. 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).


 2. You already agreed that having build time options for performance reasons is fine, why change now?
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
Other DPDK libraries provide stats based on build time configuration: Ethdev, QoS scheduler, IP fragmentation. Are you suggesting we need to remove the build time stats option for these libraries, too? 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.

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.


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?
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. I personally think the stats support 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_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.