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

[Pattan, Reshma]

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>

Thanks,
Reshma