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

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

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, May 26, 2015 10:48 PM
> To: Dumitrescu, Cristian
> Cc: Gajdzica, MaciejX T; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
> ports and tables
> 
> On Tue, 26 May 2015 21:35:22 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > > Hemminger
> > > Sent: Tuesday, May 26, 2015 3:57 PM
> > > To: Gajdzica, MaciejX T
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for
> librte_pipeline
> > > ports and tables
> > >
> > > On Tue, 26 May 2015 15:39:18 +0200
> > > Maciej Gajdzica <maciejx.t.gajdzica@intel.com> wrote:
> > >
> > > > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > > > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > > > +	({ (counter) += (val); })
> > > > +
> > > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > > > +	({ (counter) += __builtin_popcountll(mask); })
> > > > +#else
> > > > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > > > +#endif
> > >
> > > This is worse idea than the previous one.
> > > I want statistics done on a per lcore basis, and always available
> > > because real users on production system want statistics (but they
> > > don't want log spam).
> >
> > Stephen,
> >
> > Thomas and myself converged towards this solution, Thomas asked if
> anybody objects, you did not (http://www.dpdk.org/ml/archives/dev/2015-
> May/018099.html) . You say this idea is bad, but what exactly is your
> objection? Do you have an alternative proposal?
> 
> Yes. Always keep statistics.
> 
> We use functions like this internally.
> 
> struct xxx_stats {
> 	uint64_t mib[XXX_MIB_MAX];
> };
> extern struct xxx_stats xxx_stats[RTE_MAX_LCORE];
> 
> #define XXXSTAT_INC(type)	xxxstats[rte_lcore_id()].mibs[type]++
> 
> 

This can only be applied if stats are always enabled. I know this is your preferred choice, but other people have the requirement to be able to have the stats collection configurable, and we should also accommodate the needs of those people. Your code snippet does not provide a solution for this problem.

> > You already mentioned in the previous thread you would like to have per
> lcore statistics. I was kindly asking you to describe your idea, but you did not
> do this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ).
> Can you please describe it? Each port instance has its own statistics counters.
> Each lcore can run one or more pipeline instances, therefore each lcore
> typically runs several port instances of the same or different type (each port
> instance with its own statistics), so how is "per lcore stats" requirement
> applicable here?
> 
> I thought you were familiar with technique of having per-cpu structures and
> variables
> widely used in Linux kernel to prevent cache thrashing. Although you call it
> false sharing,
> it isn't false., it happens when same counter ping-pongs between multiple
> threads for
> no added benefit.

The "per lcore stats" is applicable to stats structures that are accessed by more than one lcore. In our case, each port instance is handled by a single lcore, so it is never the case that two lcores would access the stats of the same port instance. So, we can safely say that port stats are "per lcore" already.

> 
> 
> > You also reiterate that you would like to have the stats always enabled. You
> can definitely do this, it is one of the available choices, but why not also
> accommodate the users that want to pick the opposite choice? Why force
> apps to spend cycles on stats if the app either does not want these counters
> (library counters not relevant for that app, maybe the app is only interested
> in maintaining some other stats that it implements itself) or do not want
> them anymore (maybe they only needed them during debug phase), etc?
> Jay asked this question, and I did my best in my reply to describe our
> motivation (http://www.dpdk.org/ml/archives/dev/2015-May/017992.html).
> Maybe you missed that post, it would be good to get your reply on this one
> too.
> 
> I want to see DPDK get out of the config madness.
> This is real code, not an Intel benchmark special.

You seem to make this assumption that our real reason for having the stats collection configurable is to get good benchmark results by disabling the stats. This cannot be a real reason, as a complete benchmark reports always specifies if stats were enabled or not. Easy to check.

By stating this, you implicitly acknowledge that stats collection consumes CPU cycles that might be significant for the overall app performance, right? So why waste CPU cycles in the case when the app does not need those counters?

The message I am trying to get across to you is that there are multiple _real_ reasons why an app would not want to collect the stats counters of library X, considering that stats counters do not come for free, but at the expense of CPU cycles:

1. For app A, the stats counters of library X might be totally irrelevant. Maybe the app does not care how many packets got through/got dropped by port Y of pipeline instance Z. Maybe the app has some other stats that it cares about, e.g. number of online users, mean connection time, etc that are collected by the app, not the library, therefore counters collected by library X are meaningless and they should not be collected;

2. For app B, the library stats are only relevant during debugging phase; once debugging completed, stats are no longer required.

Why do you want to have the library forcing the app to collect some counters that the app might not want to collect? Let's not force people to do what we think is right, our job here is to provide options for them to pick.