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 11:02 PM
> To: Stephen Hemminger; Dumitrescu, Cristian
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] pipeline: add statistics for librte_pipeline
> ports and tables
> 
> 2015-05-20 10:59, Stephen Hemminger:
> > On Wed, 20 May 2015 16:44:35 +0200
> > Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> >
> > > 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.
> > >
> >
> > In my experience, per-queue or per-cpu statistics have no visible
> performance
> > impact. And every real world deployment wants statistics
> 
> Yes. Maybe that having a benchmark with per-cpu stats would help to discuss
> the first solution (stats always enabled).

What are per-cpu stats? Are you referring to the false sharing problem when each cpu core maintains its own stats, but stats from several cores end up being stores in the same cache line? We don't have this problem here.