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.
