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

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, May 21, 2015 8:30 AM
> To: Dumitrescu, Cristian
> Cc: Wodkowski, PawelX; dev at dpdk.org; Jastrzebski, MichalX K
> Subject: Re: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pipeline
> ports and tables
> 
> 2015-05-20 23:41, Dumitrescu, Cristian:
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > 2015-05-20 13:57, Dumitrescu, Cristian:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > > 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:
> > > > > > 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)
> > > >
> > > > 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?
> > >
> > > I think that counting stats is a kind of logging.
> > > Some stats are always counted (see drivers) and you want to use these
> ones
> > > only for debugging (after rebuilding DPDK with some debug options).
> > > So I suggest, as second solution, to check CONFIG_RTE_LOG_LEVEL is at
> > > debug
> > > level instead of having one option per module.
> > > It would be implemented with "#if RTE_LOG_LEVEL == RTE_LOG_DEBUG"
> in
> > > RTE_PIPELINE_STATS_ADD.
> >
> > The problem I see with this approach is that you cannot turn off debug
> > messages while still having the statistics collected.  We should allow
> > people to collects the stats without getting the debug messages. How do
> > we do this?
> 
> By setting build-time log level to debug, you would enable stats and debug
> messages. Then you can adjust the log messages level with the run-time
> option --log-level.

This is a really clever trick! I have to say it took me some time to make sure I got it right :)
So when RTE_LOG_LEVEL (build time option) is set to DEBUG (i.e. 8), then we get both the stats and the debug messages, but then we can adjust the level of debug messages at run-time through the --log-level EAL option.
I can work with this option, thank you!

There is one more simple proposal that came to my mind late last night: how about single config file option RTE_COLLECT_STATS=y/n that should be used by all the libs across the board to indicate whether stats should be enabled or not?
This is logically very similar to the solution above, as they both look at a single option in the config file, but maybe it is also more intuitive for people.

I will go with your choice. Which one do you pick?