[v4] lib/metrics: add unregister api for metrics

Message ID 1550849955-15101-1-git-send-email-wan.junjie@foxmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v4] lib/metrics: add unregister api for metrics |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

=?ISO-8859-1?B?V2FuIEp1bmppZQ==?= Feb. 22, 2019, 3:39 p.m. UTC
  From: junka <wan.junjie@foxmail.com>

The bitmap will help maintain the metrics. We can dynamically
add and remove metrics data. For example, after uninit latency lib,
it could remove itself from the metrics. This could make the result
from rte_metrics_get_names much more simple to display the wanted
metrics data only.

Signed-off-by: junka <wan.junjie@foxmail.com>
---
 lib/librte_metrics/rte_metrics.c | 182 ++++++++++++++++++++++++++++-----------
 lib/librte_metrics/rte_metrics.h |  21 +++++
 2 files changed, 152 insertions(+), 51 deletions(-)
  

Comments

Remy Horton Feb. 26, 2019, 4:10 p.m. UTC | #1
This patch has checkpatch errors that will need to be fixed:

ERROR:SPACING: space prohibited before that close parenthesis ')'
#179: FILE: lib/librte_metrics/rte_metrics.c:233:
+       if (count < 1 )

ERROR:TRAILING_WHITESPACE: trailing whitespace
#242: FILE: lib/librte_metrics/rte_metrics.c:298:
+^I^Ifor (idx_name = 0; idx < stats->cnt_stats && $


If the patch is applied, two of the unit-tests for metrics break:

# ./test/build/app/test
RTE>>metrics_autotest
  + ------------------------------------------------------- +
  + Test Suite : Metrics Unit Test Suite
  + ------------------------------------------------------- +
  + TestCase [ 0] : test_metrics_without_init succeeded
  + TestCase [ 1] : test_metrics_reg_name_with_validname succeeded
  + TestCase [ 2] : test_metrics_reg_names succeeded
  + TestCase [ 3] : test_metrics_update_value failed
  + TestCase [ 4] : test_metrics_update_values failed
  + TestCase [ 5] : test_metrics_get_names succeeded
  + TestCase [ 6] : test_metrics_get_values succeeded
  + ------------------------------------------------------- +
  + Test Suite Summary
  + Tests Total :        7
  + Tests Skipped :      0
  + Tests Executed :     7
  + Tests Unsupported:   0
  + Tests Passed :       5
  + Tests Failed :       2
  + ------------------------------------------------------- +


Both of these issues will need to be addressed.


On 22/02/2019 15:39, wanjunjie wrote:
> From: junka <wan.junjie@foxmail.com>
>
> The bitmap will help maintain the metrics. We can dynamically
> add and remove metrics data. For example, after uninit latency lib,
> it could remove itself from the metrics. This could make the result
> from rte_metrics_get_names much more simple to display the wanted
> metrics data only.
>
> Signed-off-by: junka <wan.junjie@foxmail.com>
> ---
>  lib/librte_metrics/rte_metrics.c | 182 ++++++++++++++++++++++++++++-----------
>  lib/librte_metrics/rte_metrics.h |  21 +++++
>  2 files changed, 152 insertions(+), 51 deletions(-)

<snip>
  

Patch

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index 99a96b6..767baf4 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -12,6 +12,7 @@ 
 #include <rte_lcore.h>
 #include <rte_memzone.h>
 #include <rte_spinlock.h>
+#include <rte_bitmap.h>
 
 #define RTE_METRICS_MAX_METRICS 256
 #define RTE_METRICS_MEMZONE_NAME "RTE_METRICS"
@@ -28,10 +29,7 @@  struct rte_metrics_meta_s {
 	uint64_t value[RTE_MAX_ETHPORTS];
 	/** Used for global metrics */
 	uint64_t global_value;
-	/** Index of next root element (zero for none) */
-	uint16_t idx_next_set;
-	/** Index of next metric in set (zero for none) */
-	uint16_t idx_next_stat;
+
 };
 
 /**
@@ -43,14 +41,12 @@  struct rte_metrics_meta_s {
  * processes is not guaranteed.
  */
 struct rte_metrics_data_s {
-	/**   Index of last metadata entry with valid data.
-	 * This value is not valid if cnt_stats is zero.
-	 */
-	uint16_t idx_last_set;
 	/**   Number of metrics. */
 	uint16_t cnt_stats;
 	/** Metric data memory block. */
 	struct rte_metrics_meta_s metadata[RTE_METRICS_MAX_METRICS];
+	/** Metric data bitmap in use */
+	struct rte_bitmap *bits;
 	/** Metric data access lock */
 	rte_spinlock_t lock;
 };
@@ -60,6 +56,8 @@  struct rte_metrics_data_s {
 {
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
+	uint32_t bmp_size;
+	void *bmp_mem;
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return;
@@ -73,6 +71,21 @@  struct rte_metrics_data_s {
 		rte_exit(EXIT_FAILURE, "Unable to allocate stats memzone\n");
 	stats = memzone->addr;
 	memset(stats, 0, sizeof(struct rte_metrics_data_s));
+
+	bmp_size =
+		rte_bitmap_get_memory_footprint(RTE_METRICS_MAX_METRICS);
+	bmp_mem = rte_malloc("metrics_bits", bmp_size,
+						RTE_CACHE_LINE_SIZE);
+	if (bmp_mem == NULL)
+		rte_exit(EXIT_FAILURE, "Failed to allocate metrics bitmap\n");
+
+	stats->bits = rte_bitmap_init(RTE_METRICS_MAX_METRICS,
+			bmp_mem, bmp_size);
+	if (stats->bits == NULL) {
+		rte_exit(EXIT_FAILURE, "Failed to init metrics bitmap\n");
+		rte_free(bmp_mem);
+	}
+
 	rte_spinlock_init(&stats->lock);
 }
 
@@ -90,8 +103,8 @@  struct rte_metrics_data_s {
 	struct rte_metrics_meta_s *entry = NULL;
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
-	uint16_t idx_name;
-	uint16_t idx_base;
+	uint16_t idx_name, idx;
+	uint16_t idx_base = RTE_METRICS_MAX_METRICS;
 
 	/* Some sanity checks */
 	if (cnt_names < 1 || names == NULL)
@@ -110,19 +123,32 @@  struct rte_metrics_data_s {
 
 	rte_spinlock_lock(&stats->lock);
 
-	/* Overwritten later if this is actually first set.. */
-	stats->metadata[stats->idx_last_set].idx_next_set = stats->cnt_stats;
-
-	stats->idx_last_set = idx_base = stats->cnt_stats;
-
-	for (idx_name = 0; idx_name < cnt_names; idx_name++) {
-		entry = &stats->metadata[idx_name + stats->cnt_stats];
-		strlcpy(entry->name, names[idx_name], RTE_METRICS_MAX_NAME_LEN);
+	/* search for a continuous array, fail if not enough*/
+	for (idx_name = 0; idx_name < RTE_METRICS_MAX_METRICS; idx_name++) {
+		if (!rte_bitmap_get(stats->bits, idx_name)) {
+			idx_base = idx_name;
+			if (idx_base + cnt_names > RTE_METRICS_MAX_METRICS)
+				return -ENOMEM;
+			for (idx = idx_base; idx < idx_base + cnt_names; idx++) {
+				if (rte_bitmap_get(stats->bits, idx))
+					break;
+			}
+			if (idx == idx_base + cnt_names) {
+				break;
+			}
+			idx_name = idx;
+		}
+	}
+	if (idx_base == RTE_METRICS_MAX_METRICS) {
+		return -ENOMEM;
+	}
+	for (idx = idx_base; idx < idx_base + cnt_names; idx++) {
+		rte_bitmap_set(stats->bits, idx);
+		entry = &stats->metadata[idx];
+		strlcpy(entry->name, names[idx-idx_base], RTE_METRICS_MAX_NAME_LEN);
 		memset(entry->value, 0, sizeof(entry->value));
-		entry->idx_next_stat = idx_name + stats->cnt_stats + 1;
+		entry->global_value = 0;
 	}
-	entry->idx_next_stat = 0;
-	entry->idx_next_set = 0;
 	stats->cnt_stats += cnt_names;
 
 	rte_spinlock_unlock(&stats->lock);
@@ -142,7 +168,6 @@  struct rte_metrics_data_s {
 	const uint64_t *values,
 	uint32_t count)
 {
-	struct rte_metrics_meta_s *entry;
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
 	uint16_t idx_metric;
@@ -163,18 +188,14 @@  struct rte_metrics_data_s {
 
 	rte_spinlock_lock(&stats->lock);
 
-	if (key >= stats->cnt_stats) {
-		rte_spinlock_unlock(&stats->lock);
-		return -EINVAL;
-	}
 	idx_metric = key;
 	cnt_setsize = 1;
-	while (idx_metric < stats->cnt_stats) {
-		entry = &stats->metadata[idx_metric];
-		if (entry->idx_next_stat == 0)
+	while (idx_metric < RTE_METRICS_MAX_METRICS) {
+		if (rte_bitmap_get(stats->bits, idx_metric)) {
+			cnt_setsize++;
+			idx_metric++;
+		} else
 			break;
-		cnt_setsize++;
-		idx_metric++;
 	}
 	/* Check update does not cross set border */
 	if (count > cnt_setsize) {
@@ -194,17 +215,72 @@  struct rte_metrics_data_s {
 			stats->metadata[idx_metric].value[port_id] =
 				values[idx_value];
 		}
+
 	rte_spinlock_unlock(&stats->lock);
 	return 0;
 }
 
 int
+rte_metrics_unreg_values(uint16_t key, uint16_t count)
+{
+	struct rte_metrics_data_s *stats;
+	const struct rte_memzone *memzone;
+	uint16_t idx_metric;
+	uint16_t idx_value;
+	uint16_t cnt_setsize;
+
+	/* Some sanity checks */
+	if (count < 1 )
+		return -EINVAL;
+
+	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
+	if (memzone == NULL)
+		return -EIO;
+
+	stats = memzone->addr;
+	if (stats->cnt_stats < count)
+		return -EINVAL;
+
+	if (key >= RTE_METRICS_MAX_METRICS)
+		return -EINVAL;
+
+	rte_spinlock_lock(&stats->lock);
+
+	idx_metric = key;
+	cnt_setsize = 1;
+	while (idx_metric < RTE_METRICS_MAX_METRICS) {
+		if (rte_bitmap_get(stats->bits, idx_metric)) {
+			cnt_setsize++;
+			idx_metric++;
+		} else
+			break;
+	}
+	/* Check update does not cross set border */
+	if (count > cnt_setsize) {
+		rte_spinlock_unlock(&stats->lock);
+		return -ERANGE;
+	}
+
+	for (idx_value = 0; idx_value < count; idx_value++) {
+		idx_metric = key + idx_value;
+		memset(stats->metadata[idx_metric].name, 0,
+			RTE_METRICS_MAX_NAME_LEN);
+		rte_bitmap_clear(stats->bits, idx_metric);
+	}
+	stats->cnt_stats -= count;
+	rte_spinlock_unlock(&stats->lock);
+
+	return 0;
+}
+
+
+int
 rte_metrics_get_names(struct rte_metric_name *names,
 	uint16_t capacity)
 {
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
-	uint16_t idx_name;
+	uint16_t idx_name, idx = 0;
 	int return_value;
 
 	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
@@ -219,10 +295,15 @@  struct rte_metrics_data_s {
 			rte_spinlock_unlock(&stats->lock);
 			return return_value;
 		}
-		for (idx_name = 0; idx_name < stats->cnt_stats; idx_name++)
-			strlcpy(names[idx_name].name,
-				stats->metadata[idx_name].name,
-				RTE_METRICS_MAX_NAME_LEN);
+		for (idx_name = 0; idx < stats->cnt_stats && 
+			idx_name < RTE_METRICS_MAX_METRICS; idx_name++) {
+			if (rte_bitmap_get(stats->bits, idx_name)) {
+				strlcpy(names[idx].name,
+					stats->metadata[idx_name].name,
+					RTE_METRICS_MAX_NAME_LEN);
+				idx++;
+			}
+		}
 	}
 	return_value = stats->cnt_stats;
 	rte_spinlock_unlock(&stats->lock);
@@ -237,7 +318,7 @@  struct rte_metrics_data_s {
 	struct rte_metrics_meta_s *entry;
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
-	uint16_t idx_name;
+	uint16_t idx_name, idx = 0;
 	int return_value;
 
 	if (port_id != RTE_METRICS_GLOBAL &&
@@ -257,24 +338,23 @@  struct rte_metrics_data_s {
 			rte_spinlock_unlock(&stats->lock);
 			return return_value;
 		}
-		if (port_id == RTE_METRICS_GLOBAL)
-			for (idx_name = 0;
-					idx_name < stats->cnt_stats;
-					idx_name++) {
-				entry = &stats->metadata[idx_name];
-				values[idx_name].key = idx_name;
-				values[idx_name].value = entry->global_value;
-			}
-		else
-			for (idx_name = 0;
-					idx_name < stats->cnt_stats;
-					idx_name++) {
+
+		for (idx_name = 0; idx < stats->cnt_stats &&
+				idx_name < RTE_METRICS_MAX_METRICS;
+				idx_name++) {
+			if (rte_bitmap_get(stats->bits, idx_name)) {
 				entry = &stats->metadata[idx_name];
-				values[idx_name].key = idx_name;
-				values[idx_name].value = entry->value[port_id];
+				values[idx].key = idx_name;
+				if (port_id == RTE_METRICS_GLOBAL)
+					values[idx].value = entry->global_value;
+				else
+					values[idx].value = entry->value[port_id];
+				idx++;
 			}
+		}
 	}
 	return_value = stats->cnt_stats;
 	rte_spinlock_unlock(&stats->lock);
+
 	return return_value;
 }
diff --git a/lib/librte_metrics/rte_metrics.h b/lib/librte_metrics/rte_metrics.h
index 67a60fa..afde1c0 100644
--- a/lib/librte_metrics/rte_metrics.h
+++ b/lib/librte_metrics/rte_metrics.h
@@ -123,6 +123,27 @@  struct rte_metric_value {
 int rte_metrics_reg_names(const char * const *names, uint16_t cnt_names);
 
 /**
+ * Unregister set of metrics.
+ *
+ * Remove the metrics previously registered
+ *
+ * @param key
+ *   Id of metrics to remove
+ *
+ * @param count
+ *   Number of metrics
+ *
+ * @return
+ *  - Zero: Success
+ *  - -EIO: Error, unable to access metrics shared memory
+ *    (rte_metrics_init() not called)
+ *  - -EINVAL: Error, invalid parameters
+ *  - -ERANGE: Error, oversized
+ */
+int
+rte_metrics_unreg_values(uint16_t key, uint16_t count);
+
+/**
  * Get metric name-key lookup table.
  *
  * @param names