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

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

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, May 20, 2015 3:45 PM
> 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
> 
> Please Cristian, do not top post.
> I'm replacing your comment in the right context.
> 
> 2015-05-20 13:57, Dumitrescu, Cristian:
> > From: Thomas Monjalon [mailto:thomas.monjalon@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?

Although most people use the stats for debugging and then turn them off, some other people are OK with the stats related performance drop for the production code (e.g. they might want to log some stats periodically), but they want to get rid of the debug messages. 

Potential idea:
Would it be acceptable to create a new log level that is dedicated to stats? This would have to be higher priority than all the present ones, so it needs to be: #define RTE_LOG_STATS    1U. This way, we can get rid of all the debug messages while still logging stats:

#if ((RTE_LOG_STATS <= RTE_LOG_LEVEL)
#define STATS_COUNTER_ADD(level, type, counter, val) counter += val
#else
#define STATS_COUNTER_ADD(level, type, counter, val)
#endif

However, this won't allow getting the debug messages with stats collection being suppressed. But probably this case is not relevant.