Re: [dpdk-dev] [PATCH] doc: guidelines for library statistics

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

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, June 11, 2015 1:05 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] doc: guidelines for library statistics
> 
> Hi Cristian,
> 
> Thanks for trying to make a policy clearer.
> We need to make a decision in the coming week.
> Below are comments on the style and content.
> 
> 2015-06-08 15:50, Cristian Dumitrescu:
> >  doc/guides/guidelines/statistics.rst | 42
> ++++++++++++++++++++++++++++++++++++
> 
> Maybe we should have a more general file like design.rst.

I am not sure I correctly understood your suggestion. Do you want the section on statistics to be called design.rst? Do you want it to be part of doc/guides/guidelines or create a brand new document?
To me, the initial idea of doc/guidelines/statistics.rst makes more sense, but I am fine either way.

> In order to have a lot of readers of such guidelines, they must be concise.

I reordered the sections to provide the guidelines first and the motivation afterwards. I think it is good to keep the motivation together with the guidelines. Do you suggest to remove the motivation from this document?

> 
> Please wrap lines to be not too long and/or split lines after the end of a
> sentence.

Done in next version.

> 
> > +Library Statistics
> > +==================
> > +
> > +Description
> > +-----------
> > +
> > +This document describes the guidelines for DPDK library-level statistics
> counter support. This includes guidelines for turning library statistics on and
> off, requirements for preventing ABI changes when library statistics are
> turned on and off, etc.
> 
> Should we consider that driver stats and lib stats are different in DPDK? Why?

I did update the document to make it clear these guidelines are applicable for the case when the stats counters are maintained by the library itself.
I think the primary difference is whether the stats counters are implemented by the HW (e.g. NIC) or in SW by the CPU.
* In the first case, the CPU does not spend cycles maintaining the counters. System resources might be consumed by the NIC for maintaining the counters (e.g. memory bandwidth), so sometimes the NICs provide mechanisms to enable/disable stats, which is done as part of init code rather than buid time.
* In the second case, the CPU maintains the counters, which comes at the usual cost of cycles and system resources. These guidelines focus on this case.

> 
> > +Motivation to allow the application to turn library statistics on and off
> > +-------------------------------------------------------------------------
> > +
> > +It is highly recommended that each library provides statistics counters to
> allow the application to monitor the library-level run-time events. Typical
> counters are: number of packets received/dropped/transmitted, number of
> buffers allocated/freed, number of occurrences for specific events, etc.
> > +
> > +Since the resources consumed for library-level statistics counter collection
> have to be spent out of the application budget and the counters collected by
> some libraries might not be relevant for the current application, in order to
> avoid any unwanted waste of resources and/or performance for the
> application, the application is to decide at build time whether the collection
> of library-level statistics counters should be turned on or off for each library
> individually.
> 
> It would be good to have acknowledgements or other opinions on this.
> Some of them were expressed in other threads. Please comment here.
> 
> > +Library-level statistics counters can be relevant or not for specific
> applications:
> > +* For application A, counters maintained by library X are always relevant
> and the application needs to use them to implement certain features, as
> traffic accounting, logging, application-level statistics, etc. In this case, the
> application requires that collection of statistics counters for library X is always
> turned on;
> > +* For application B, counters maintained by library X are only useful during
> the application debug stage and not relevant once debug phase is over. In
> this case, the application may decide to turn on the collection of library X
> statistics counters during the debug phase and later on turn them off;
> 
> Users of binary package have not this choice.

There is a section in the document about allowing stats to be turned on/off without ABI impact.

> 
> > +* For application C, counters maintained by library X are not relevant at all.
> It might me that the application maintains its own set of statistics counters
> that monitor a different set of run-time events than library X (e.g. number of
> connection requests, number of active users, etc). It might also be that
> application uses multiple libraries (library X, library Y, etc) and it is interested
> in the statistics counters of library Y, but not in those of library X. In this case,
> the application may decide to turn the collection of statistics counters off for
> library X and on for library Y.
> > +
> > +The statistics collection consumes a certain amount of CPU resources
> (cycles, cache bandwidth, memory bandwidth, etc) that depends on:
> > +* Number of libraries used by the current application that have statistics
> counters collection turned on;
> > +* Number of statistics counters maintained by each library per object type
> instance (e.g. per port, table, pipeline, thread, etc);
> > +* Number of instances created for each object type supported by each
> library;
> > +* Complexity of the statistics logic collection for each counter: when only
> some occurrences of a specific event are valid, several conditional branches
> might involved in the decision of whether the current occurrence of the
> event should be counted or not (e.g. on the event of packet reception, only
> TCP packets with destination port within a certain range should be recorded),
> etc.
> > +
> > +Mechanism to allow the application to turn library statistics on and off
> > +------------------------------------------------------------------------
> > +
> > +Each library that provides statistics counters should provide a single build
> time flag that decides whether the statistics counter collection is enabled or
> not for this library. This flag should be exposed as a variable within the DPDK
> configuration file. When this flag is set, all the counters supported by current
> library are collected; when this flag is cleared, none of the counters
> supported by the current library are collected:
> > +
> > +	#DPDK file “./config/common_linuxapp”,
> “./config/common_bsdapp”, etc
> > +	CONFIG_RTE_LIBRTE_<LIBRARY_NAME>_COLLECT_STATS=y/n
> 
> Why not simply CONFIG_RTE_LIBRTE_<LIBRARY_NAME>_STATS (without
> COLLECT)?
> 

The reason I propose the name STATS_COLLECT rather than just STATS is to give a strong hint that counters are always allocated and the stats API is always available, while just the collection/update of the counters is configurable. The simple STATS mention might drive some library developers to make the stats API compiled in and out, which is something we want to prevent in order to avoid ABI changes. I don't have a strong preference though, if you still think STATS is better than STATS_COLLECT, let me know.

> > +The default value for this DPDK configuration file variable (either “yes” or
> “no”) is left at the decision of each library.
> > +
> > +Prevention of ABI changes due to library statistics support
> > +-----------------------------------------------------------
> > +
> > +The layout of data structures and prototype of functions that are part of
> the library API should not be affected by whether the collection of statistics
> counters is turned on or off for the current library. In practical terms, this
> means that space is always allocated in the API data structures for statistics
> counters and the statistics related API functions are always built into the
> code, regardless of whether the statistics counter collection is turned on or
> off for the current library.
> > +
> > +When the collection of statistics counters for the current library is turned
> off, the counters retrieved through the statistics related API functions should
> have the default value of zero.