Re: [PATCH v6 1/4] lib: add information metrics library

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2017-01-12 00:03, Remy Horton:
> 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.

[...]
> --- a/doc/api/doxy-api.conf
> +++ b/doc/api/doxy-api.conf
> @@ -58,6 +58,7 @@ INPUT                   = doc/api/doxy-api-index.md \
>                            lib/librte_reorder \
>                            lib/librte_ring \
>                            lib/librte_sched \
> +                          lib/librte_metrics \
>                            lib/librte_table \
>                            lib/librte_timer \
>                            lib/librte_vhost

It is not in the right order.
Tip: when you add an item to a list, you should ask yourself what is the
order. There are 3 types of order for the lists in DPDK:
	- chronological (add at the end)
	- alphabetical
	- logical/semantic
The game is to find the right one :)

[...]
> @@ -171,6 +177,7 @@ The libraries prepended with a plus sign were incremented in this version.
>       librte_mbuf.so.2
>       librte_mempool.so.2
>       librte_meter.so.1
> +   + librte_metrics.so.1
>       librte_net.so.1
>       librte_pdump.so.1
>       librte_pipeline.so.3

Right order here ;)

[...]
> --- /dev/null
> +++ b/lib/librte_metrics/rte_metrics.h
> +/** Used to indicate port-independent information */
> +#define RTE_METRICS_NONPORT -1

I do not understand this constant.
Why using the word "port" to name any device?
What means independent?

> +/**
> + * Metric name
> + */
> +struct rte_metric_name {
> +	/** String describing metric */
> +	char name[RTE_METRICS_MAX_NAME_LEN];
> +};

Why a struct for a simple string?

> +/**
> + * Metric name.

Copy/paste typo?

> + */
> +struct rte_metric_value {
> +	/** Numeric identifier of metric */
> +	uint16_t key;

How the key is bound to the name?
Remember how the xstats comments were improved:
	http://dpdk.org/commit/6d52d1d

> +	/** Value for metric */
> +	uint64_t value;
> +};
> +
> +
> +/**
> + * Initializes metric module. This only has to be explicitly called if you
> + * intend to use rte_metrics_reg_metric() or rte_metrics_reg_metrics() from a
> + * secondary process. This function must be called from a primary process.
> + */
> +void rte_metrics_init(void);
> +
> +
> +/**
> + * Register a metric

You need to explain what is implied in registering.
I have the same comment for registering a set of metrics.

[...]
> +int rte_metrics_reg_metric(const char *name);
> +
> +/**
> + * Register a set of metrics
[...]
> +int rte_metrics_reg_metrics(const char **names, uint16_t cnt_names);
> +
> +/**
> + * Get metric name-key lookup table.
> + *
> + * @param names
> + *   Array of names to receive key names
> + *
> + * @param capacity
> + *   Space available in names

What happens if there is not enough space?

> + * @return
> + *   - Non-negative: Success (number of names)
> + *   - Negative: Failure
> + */
> +int rte_metrics_get_names(
> +	struct rte_metric_name *names,
> +	uint16_t capacity);