[dpdk-dev] [PATCH v3 1/3] lib: add information metrics library

Pattan, Reshma reshma.pattan at intel.com
Mon Nov 7 16:25:43 CET 2016


Hi,

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Friday, November 4, 2016 4:43 PM
> To: Remy Horton <remy.horton at intel.com>
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 1/3] lib: add information metrics library
> 
> Hi,
> 
> Few comments below.
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> > Sent: Friday, November 4, 2016 3:36 AM
> > To: dev at dpdk.org
> > Subject: [dpdk-dev] [PATCH v3 1/3] lib: add information metrics
> > library
> >
> > This patch adds a new information metric library that allows other
> > modules to register named metrics and update their values. It is
> > intended to be independent of ethdev, rather than mixing ethdev and
> > non-ethdev information in xstats.
> >
> > Signed-off-by: Remy Horton <remy.horton at intel.com>
> > ---
> > +int
> > +
> > +int
> > +rte_metrics_get_values(int port_id,
> > +	struct rte_stat_value *values,
> > +	uint16_t capacity)
> > +{
> > +	struct rte_metrics_meta_s *entry;
> > +	struct rte_metrics_data_s *stats;
> > +	const struct rte_memzone *memzone;
> > +	uint16_t idx_name;
> > +	int return_value;
> > +
> > +	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
> > +	/* If not allocated, fail silently */
> > +	if (memzone == NULL)
> > +		return 0;
> > +	stats = memzone->addr;
> > +	rte_spinlock_lock(&stats->lock);
> > +
> > +	if (values != NULL) {
> > +		if (capacity < stats->cnt_stats) {
> > +			rte_spinlock_unlock(&stats->lock);
> > +			return -ERANGE;
> > +		}
> > +		for (idx_name = 0; idx_name < stats->cnt_stats; idx_name++)
> > {
> > +			entry = &stats->metadata[idx_name];
> > +			values[idx_name].key = idx_name;
> > +			values[idx_name].value = entry->value[port_id];
> > +		}
> 
> Here you also  need to include logic to return values for  port independent
> metrics.
> 
> > diff --git a/lib/librte_metrics/rte_metrics.h
> > b/lib/librte_metrics/rte_metrics.h
> > new file mode 100644
> > index 0000000..6b75404
> > --- /dev/null
> > +++ b/lib/librte_metrics/rte_metrics.h
> > @@ -0,0 +1,204 @@
> > +/**
> > + * Statistic name
> > + */
> > +struct rte_metric_name {
> > +	/** String describing statistic */
> > +	char name[RTE_METRICS_MAX_NAME_LEN]; };
> > +
> > +
> > +/**
> > + * Statistic name.
> > + */
> 
> Need to correct the description to "stats values" or "metric values."
> 
> > +struct rte_stat_value {
> 
> Need to change the name to rte_metric_value.
> 
> > +	/** Numeric identifier of statistic */
> > +	uint16_t key;
> > +	/** Value for statistic */
> > +	uint64_t value;
> > +};
> > +
> > +
> > +/**
> > + * Initialises statistic module. This only has to be explicitly
> > +called
> 
> Typo < Initialises>

To avoid confusion, here I mean to say "Initializes" should be used (i.e. US English) .
Also another non-related comment, you may need to add a comment about EMWA
near the mean bit rate calculation code.

> 
> Thanks,
> Reshma


More information about the dev mailing list