[dpdk-dev] [PATCH v7 1/6] lib: add information metrics library

Van Haaren, Harry harry.van.haaren at intel.com
Tue Jan 17 12:01:20 CET 2017


> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of 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.
> 
> Signed-off-by: Remy Horton <remy.horton at intel.com>


Comments inline below.


> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9645c9b..4a19497 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -596,6 +596,11 @@ F: lib/librte_jobstats/
>  F: examples/l2fwd-jobstats/
>  F: doc/guides/sample_app_ug/l2_forward_job_stats.rst
> 
> +Metrics
> +M: Remy Horton <remy.horton at intel.com>
> +F: lib/librte_metrics/
> +F: doc/guides/sample_app_ug/keep_alive.rst

Copy paste error, Keep alive sample app guide should not be here. There is no sample app / guide for usage of the metrics library in this patchset as it is.

> +++ b/lib/librte_metrics/rte_metrics.c
> @@ -0,0 +1,310 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   All rights reserved.

Update to 2016-2017

> +/**
> + * 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;
> +};

Nit: why the _s in rte_metrics_meta_s? Is there a reason why struct rte_metrics_meta {  is not suitable? Same question for rte_metrics_data_s.

> +void
> +rte_metrics_init(void)
> +{
> +	struct rte_metrics_data_s *stats;
> +	const struct rte_memzone *memzone;
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
> +	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
> +	if (memzone != NULL)
> +		return;
> +	memzone = rte_memzone_reserve(RTE_METRICS_MEMZONE_NAME,
> +		sizeof(struct rte_metrics_data_s), rte_socket_id(), 0);

In my opinion, passing socket_id to rte_metrics_init() would be a better API, than relying on the metrics initialization core to be on the correct socket.

> +	if (memzone == NULL)
> +		rte_exit(EXIT_FAILURE, "Unable to allocate stats memzone\n");
> +	stats = memzone->addr;
> +	memset(stats, 0, sizeof(struct rte_metrics_data_s));
> +	rte_spinlock_init(&stats->lock);

Should this spinlock be initialized before the creation of the memzone? Creating the memzone first, and later initializing the locking-structure for it feels racey to me. The lock should probably be taken and unlocked again after init and memset.

> +}
> +
> +int
> +rte_metrics_reg_metric(const char *name)
> +{

Nit: would rte_metrics_reg_name() be a better function name?

> +	const char * const list_names[] = {name};
> +
> +	return rte_metrics_reg_metrics(list_names, 1);
> +}
> +
> +int
> +rte_metrics_reg_metrics(const char * const *names, uint16_t cnt_names)
> +{

Nit: would rte_metrics_reg_names() be a better function name?

> +
> +int
> +rte_metrics_update_metric(int port_id, uint16_t key, const uint64_t value)
> +{

Would rte_metrics_update_single() be a better function name? I presume updating a single metric at a time is exception case, and the (see below) more generic rte_metrics_update() would be more commonly used.

> +	return rte_metrics_update_metrics(port_id, key, &value, 1);
> +}
> +
> +int
> +rte_metrics_update_metrics(int port_id,
> +	uint16_t key,
> +	const uint64_t *values,
> +	uint32_t count)

Would rte_metrics_update() be a better function name?

> +{
> +	struct rte_metrics_meta_s *entry;
> +	struct rte_metrics_data_s *stats;
> +	const struct rte_memzone *memzone;
> +	uint16_t idx_metric;
> +	uint16_t idx_value;
> +	uint16_t cnt_setsize;
> +
> +	if (port_id != RTE_METRICS_GLOBAL &&
> +			(port_id < 0 || port_id > RTE_MAX_ETHPORTS))
> +		return -EINVAL;
> +
> +	rte_metrics_init();

See above comments on rte_metrics_init() taking a socket_id parameter. Here any core could call update_metrics(), and if the library was not yet initialized, the memory for metrics would end up on the socket of this core. This should be avoided by
A) adding socket_id parameter to the metrics_init() function
B) Demanding that metrics_init() is called by the application before any core uses update_metrics() 

> +	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
> +	if (memzone == NULL)
> +		return -EIO;
> +	stats = memzone->addr;
> +
> +	rte_spinlock_lock(&stats->lock);
> +	idx_metric = key;
> +	cnt_setsize = 1;
> +	while (idx_metric < stats->cnt_stats) {
> +		entry = &stats->metadata[idx_metric];
> +		if (entry->idx_next_stat == 0)
> +			break;
> +		cnt_setsize++;
> +		idx_metric++;
> +	}
> +	/* Check update does not cross set border */
> +	if (count > cnt_setsize) {
> +		rte_spinlock_unlock(&stats->lock);
> +		return -ERANGE;
> +	}

What is a "set border"? I think this ensures that one set of metrics cannot overwrite the index's of another metrics set - correct?

> +
> +	if (port_id == RTE_METRICS_GLOBAL)
> +		for (idx_value = 0; idx_value < count; idx_value++) {
> +			idx_metric = key + idx_value;
> +			stats->metadata[idx_metric].nonport_value =
> +				values[idx_value];
> +		}
> +	else
> +		for (idx_value = 0; idx_value < count; idx_value++) {
> +			idx_metric = key + idx_value;
> +			stats->metadata[idx_metric].value[port_id] =
> +				values[idx_value];
> +		}
> +	rte_spinlock_unlock(&stats->lock);
> +	return 0;
> +}

<snip>

> diff --git a/lib/librte_metrics/rte_metrics.h b/lib/librte_metrics/rte_metrics.h
> new file mode 100644
> index 0000000..fd82af9
> --- /dev/null
> +++ b/lib/librte_metrics/rte_metrics.h
> @@ -0,0 +1,223 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   All rights reserved.

Add -2017

> +
> +
> +/**
> + * 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);

As noted in the C function above:
- accept a socket-id
- demand an application calls it if it will use the metrics library. 

<snip>


More information about the dev mailing list