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

[Remy Horton]

Some points addressed. Will cover other ones later.

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

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


>> +  [Device Metrics]     (@ref rte_metrics.h),
> No first letter uppercase in this list.
Fixed.

>> +     A library that allows information metrics to be added and update. It is
> update -> updated
Fixed.

> added and updated by who?
Elaborated.

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


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


>> +   + librte_bitratestats.so.1
> not part of this patch

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


>> +DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics
> insert it below librte_jobstats is a better choice

Done.


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


>> + * RTE Metrics module
> RTE is not meaningful here.
> Please prefer DPDK.

Fixed.

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


>> +/**
>> + * Global (rather than port-specific) metric.
> It does not say what kind of constant it is. A special metric id?

Yes. Elaborated.


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


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


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


>> + * Registering a metric is the way third-parties declare a parameter
> third-party? You mean the provider?

Yes.


>> + *  - \b -EINVAL: Error, invalid parameters
>> + *  - \b -ENOMEM: Error, maximum metrics reached
> Please, no extra formatting in doxygen.

Fixed.