Some points addressed. Will cover other ones later.

On 30/01/2017 15:50, Thomas Monjalon wrote:
[..]

quoted

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

Done. Rebase merging doesn't help with sorting here.

quoted

+  [Device Metrics]     (@ref rte_metrics.h),

No first letter uppercase in this list.

Fixed.

quoted

+     A library that allows information metrics to be added and update. It is

update -> updated

Fixed.

added and updated by who?

Elaborated.

quoted

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

and independent of the cryptodev library?

Yes. The aim is to have no sub-dependencies.

My original plan was to introduce some form of parameter registration 
scheme into xstats to replace the current hard-coded tables, since I 
suspected libbitrate/liblatency would not be the last additions. I 
decided to spin it out into a library in its own right, as it seemed 
cleaner than shoving a load of non-driver stuff into xstats.

Does it apply to other types of devices (cryptodev/eventdev)?

I've not been following cryptodev/eventdev, but short answer yes.

quoted

      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.

Fixed.

quoted

+   + librte_bitratestats.so.1

not part of this patch

Fixed. Artefact of sorting out a merge mess-up.

quoted

+DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics

insert it below librte_jobstats is a better choice

Done.

quoted

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

Noted. Even though Doxygen isn't required, I think it preferable to use 
a consistent style in both .c and .h files.

quoted

+ * RTE Metrics module

RTE is not meaningful here.
Please prefer DPDK.

Fixed.

quoted

+ * 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?).

Noted. Will elaborate.

What do you mean by "push model"? A callback is used?

Updating is done in response to producers having new information. In 
contrast in a pull model an update would happen in response to a polling 
by a consumer.

Originally (back in August I think) I used a pull model where producers 
would register callbacks that were called in response to 
rte_metrics_get() by a consumer, but that assumed that producers and 
consumers were within the same process. Using shared memory and making 
it ASLR-safe means not using pointers.

Aside: In this former pull model, port_id was passed verbatim to the 
producers' callbacks to interpret in whatever way they saw fit, so there 
was no inherant need to stop "magic" values outside the usual 0-255 used 
for port ids. Hence where the next thing originally came from...

quoted

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

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

Yes. Elaborated.

quoted

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

Certainly within the constant name. Don't see what's wrong with 
referring to it in description though.

quoted

+ * 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?

Good question - was going to put it down to cut'n'paste while baseing 
the descriptions on Olivier Matz's rewording, but that was for xstats..

quoted

+ * 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?

There shouldn't be any significant performance penalities, but I am not 
particularly fond in principle of initalising component libraries 
regardless of whether they are used. (Actually it was previously 
initalised on first use, but that had a race condition.)

quoted

+ * Registering a metric is the way third-parties declare a parameter

third-party? You mean the provider?

Yes.

quoted

+ *  - \b -EINVAL: Error, invalid parameters
+ *  - \b -ENOMEM: Error, maximum metrics reached

Please, no extra formatting in doxygen.

Fixed.