Re: [dpdk-dev] [PATCH v9 1/7] lib: add information metrics library

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

Hi Remy,

> 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.

I'm still not convinced by this library, and this introduction does
not help a lot.

I would like to thanks Harry for the review of this series.
If we had more opinions or enthousiasm about this patch, it would
be easier to accept this new library and assert it is the way to go.

It could be a matter of technical board discussion if we had a clear
explanation of the needs, the pros and cons of this design.

The overview for using this library should be given in the prog guide.


2017-01-18 15:05, Remy Horton:
> --- a/config/common_base
> +++ b/config/common_base
> @@ -593,3 +593,8 @@ CONFIG_RTE_APP_TEST_RESOURCE_TAR=n
>  CONFIG_RTE_TEST_PMD=y
>  CONFIG_RTE_TEST_PMD_RECORD_CORE_CYCLES=n
>  CONFIG_RTE_TEST_PMD_RECORD_BURST_STATS=n
> +
> +#
> +# Compile the device metrics library
> +#
> +CONFIG_RTE_LIBRTE_METRICS=y

I know the config file is not so well sorted.
However it would be a bit more logical below CONFIG_RTE_LIBRTE_JOBSTATS.

> diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> index 72d59b2..94f0f69 100644
> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -150,4 +150,5 @@ There are many libraries, so their headers may be grouped by topics:
>    [common]             (@ref rte_common.h),
>    [ABI compat]         (@ref rte_compat.h),
>    [keepalive]          (@ref rte_keepalive.h),
> +  [Device Metrics]     (@ref rte_metrics.h),

No first letter uppercase in this list.

> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -34,6 +34,12 @@ New Features
>  
>       Refer to the previous release notes for examples.
>  
> +   * **Added information metric library.**
> +
> +     A library that allows information metrics to be added and update. It is

update -> updated

added and updated by who?

> +     intended to provide a reporting mechanism that is independent of the
> +     ethdev library.

and independent of the cryptodev library?
Does it apply to other types of devices (cryptodev/eventdev)?

> +
>       This section is a comment. do not overwrite or remove it.
>       Also, make sure to start the actual text at the margin.
>       =========================================================

Your text should start below this line, and indented at the margin.

> @@ -205,6 +211,7 @@ The libraries prepended with a plus sign were incremented in this version.
>  .. code-block:: diff
>  
>       librte_acl.so.2
> +   + librte_bitratestats.so.1

not part of this patch

> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -58,6 +58,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_TABLE) += librte_table
>  DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += librte_pipeline
>  DIRS-$(CONFIG_RTE_LIBRTE_REORDER) += librte_reorder
>  DIRS-$(CONFIG_RTE_LIBRTE_PDUMP) += librte_pdump
> +DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics

insert it below librte_jobstats is a better choice

> --- /dev/null
> +++ b/lib/librte_metrics/rte_metrics.c
> +/**
> + * Internal stats metadata and value entry.
> + *
> + * @internal
> + * @param name
> + *   Name of metric
> + * @param value
> + *   Current value for metric
> + * @param idx_next_set
> + *   Index of next root element (zero for none)
> + * @param idx_next_metric
> + *   Index of next metric in set (zero for none)
> + *
> + * Only the root of each set needs idx_next_set but since it has to be
> + * assumed that number of sets could equal total number of metrics,
> + * having a separate set metadata table doesn't save any memory.
> + */
> +struct rte_metrics_meta_s {
> +	char name[RTE_METRICS_MAX_NAME_LEN];
> +	uint64_t value[RTE_MAX_ETHPORTS];
> +	uint64_t nonport_value;
> +	uint16_t idx_next_set;
> +	uint16_t idx_next_stat;
> +};

It would be a lot easier to read with comments near each field.
It would avoid to forget some fields like nonport_value in this struct.
You do not need to use a doxygen syntax in a .c file.

> --- /dev/null
> +++ b/lib/librte_metrics/rte_metrics.h
> +/**
> + * @file
> + *
> + * RTE Metrics module

RTE is not meaningful here.
Please prefer DPDK.

> + *
> + * Metric information is populated using a push model, where the
> + * information provider calls an update function on the relevant
> + * metrics. Currently only bulk querying of metrics is supported.
> + */

This description should explain who is a provider (drivers?) and who
is the reader (registered thread?).
What do you mean by "push model"? A callback is used?

> +/**
> + * Global (rather than port-specific) metric.

It does not say what kind of constant it is. A special metric id?

> + *
> + * When used instead of port number by rte_metrics_update_metric()
> + * or rte_metrics_update_metric(), the global metrics, which are
> + * not associated with any specific port, are updated.
> + */
> +#define RTE_METRICS_GLOBAL -1

I thought you agreed that "port" is not really a good wording.

> +/**
> + * A name-key lookup for metrics.
> + *
> + * An array of this structure is returned by rte_metrics_get_names().
> + * The struct rte_eth_stats references these names via their array index.

rte_eth_stats?

> + */
> +struct rte_metric_name {
> +	/** String describing metric */
> +	char name[RTE_METRICS_MAX_NAME_LEN];
> +};
[...]
> +/**
> + * Metric value structure.
> + *
> + * This structure is used by rte_metrics_get_values() to return metrics,
> + * which are statistics that are not generated by PMDs. It maps a name key,

Here we have a definition of what is a metric:
"statistics that are not generated by PMDs"
It could help in the introduction.

> + * which corresponds to an index in the array returned by
> + * rte_metrics_get_names().
> + */
> +struct rte_metric_value {
> +	/** Numeric identifier of metric. */
> +	uint16_t key;
> +	/** Value for metric */
> +	uint64_t value;
> +};
> +
> +
> +/**
> + * Initializes metric module. This function must be called from
> + * a primary process before metrics are used.

Why not integrating it in the global init?
Is there some performance drawbacks?

> + *
> + * @param socket_id
> + *   Socket to use for shared memory allocation.
> + */
> +void rte_metrics_init(int socket_id);
> +
> +/**
> + * Register a metric, making it available as a reporting parameter.
> + *
> + * Registering a metric is the way third-parties declare a parameter

third-party? You mean the provider?

> + * that they wish to be reported. Once registered, the associated
> + * numeric key can be obtained via rte_metrics_get_names(), which
> + * is required for updating said metric's value.
> + *
> + * @param name
> + *   Metric name
> + *
> + * @return
> + *  - Zero or positive: Success (index key of new metric)
> + *  - \b -EIO: Error, unable to access metrics shared memory
> + *    (rte_metrics_init() not called)
> + *  - \b -EINVAL: Error, invalid parameters
> + *  - \b -ENOMEM: Error, maximum metrics reached

Please, no extra formatting in doxygen.

> + */
> +int rte_metrics_reg_name(const char *name);
> +