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);
