Change in libosmocore[master]: stat_item: make value ids item specific

osmith gerrit-no-reply at lists.osmocom.org
Fri Mar 26 17:11:51 UTC 2021


osmith has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/23508 )


Change subject: stat_item: make value ids item specific
......................................................................

stat_item: make value ids item specific

Fix counting of values missed because of FIFO overflow in
osmo_stat_item_get_next(), by assigning a new item value id effectively
as item->value[n + 1].id = item->value[n].id + 1, instead of increasing
a global_value_id that is shared between all items and groups. With
global_value_id, the count of values missed was wrong for one item, as
soon as a new value was added to another item.

This partially reverts b27b352e ("stats: Use a global index for stat
item values") from 2015, right after stats was added to libosmocore. It
was supposed to make multiple readers (reporters) possible, which could
read independently from stat_item (and later added comments explain it
like that). But this remained unused, stats has implemented multiple
reporters by reading all stat_items once and sending the same data to
all enabled reporters. The patch caused last_value_index in struct
osmo_stat_item to always remain at -1.

Replace this unused last_value_index with stats_next_id, so stats can
store the item-specific next_id in the struct again. It appears that
stats is the only direct user of osmo_stat_item, but if there are
others, they can bring their own item-specific next_id: functions in
stat_item.c still accept a next_id argument.

Related: OS#5088
Change-Id: Ie65dcdf52c8fc3d916e20d7f0455f6223be6b64f
---
M include/osmocom/core/stat_item.h
M src/stat_item.c
M src/stats.c
M tests/stats/stats_test.c
4 files changed, 44 insertions(+), 44 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/08/23508/1

diff --git a/include/osmocom/core/stat_item.h b/include/osmocom/core/stat_item.h
index 4a19465..29e35ef 100644
--- a/include/osmocom/core/stat_item.h
+++ b/include/osmocom/core/stat_item.h
@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <osmocom/core/defs.h>
 #include <osmocom/core/linuxlist.h>
 
 struct osmo_stat_item_desc;
@@ -23,9 +24,11 @@
 struct osmo_stat_item {
 	/*! back-reference to the item description */
 	const struct osmo_stat_item_desc *desc;
-	/*! the index of the freshest value */
-	int32_t last_value_index;
-	/*! offset to the freshest value in the value FIFO */
+	/* internal use by stats API (stats.c): the id of the next value to
+	 * be read from the FIFO. If accessing osmo_stat_item directly, without
+	 * the stats API, store this value elsewhere. */
+	int32_t stats_next_id;
+	/*! the index of the last value written to the FIFO */
 	int16_t last_offs;
 	/*! value FIFO */
 	struct osmo_stat_item_value values[0];
@@ -98,7 +101,8 @@
 
 int osmo_stat_item_discard(const struct osmo_stat_item *item, int32_t *next_id);
 
-int osmo_stat_item_discard_all(int32_t *next_id);
+int osmo_stat_item_discard_all(int32_t *next_id)
+	OSMO_DEPRECATED("Use osmo_stat_item_discard with item-specific next_id instead");
 
 typedef int (*osmo_stat_item_handler_t)(
 	struct osmo_stat_item_group *, struct osmo_stat_item *, void *);
diff --git a/src/stat_item.c b/src/stat_item.c
index d4f3b72..f5df5b6 100644
--- a/src/stat_item.c
+++ b/src/stat_item.c
@@ -37,14 +37,15 @@
  *
  *  The only supported value type is an int32_t.
  *
- *  Getting values from the osmo_stat_item does not modify its state to
- *  allow for multiple independent back-ends retrieving values (e.g. VTY
- *  and statd).
+ *  Getting values from osmo_stat_item is usually done at a high level
+ *  through the stats API (stats.c). It uses item->stats_next_id to
+ *  store what has been sent to all enabled reporters. It is also
+ *  possible to read from osmo_stat_item directly, without modifying
+ *  its state, by storing next_id outside of osmo_stat_item.
  *
  *  Each value stored in the FIFO of an osmo_stat_item has an associated
- *  value_id.  The value_id is derived from an application-wide globally
- *  incrementing counter, so (until the counter wraps) more recent
- *  values will have higher values.
+ *  value_id.  The value_id is increased with each value, so (until the
+ *  counter wraps) more recent values will have higher values.
  *
  *  When a new value is set, the oldest value in the FIFO gets silently
  *  overwritten.  Lost values are skipped when getting values from the
@@ -74,14 +75,13 @@
 
 #include <osmocom/core/utils.h>
 #include <osmocom/core/linuxlist.h>
+#include <osmocom/core/logging.h>
 #include <osmocom/core/talloc.h>
 #include <osmocom/core/timer.h>
 #include <osmocom/core/stat_item.h>
 
 /*! global list of stat_item groups */
 static LLIST_HEAD(osmo_stat_item_groups);
-/*! counter for assigning globally unique value identifiers */
-static int32_t global_value_id = 0;
 
 /*! talloc context from which we allocate */
 static void *tall_stat_item_ctx;
@@ -146,7 +146,7 @@
 
 		group->items[item_idx] = item;
 		item->last_offs = desc->item_desc[item_idx].num_values - 1;
-		item->last_value_index = -1;
+		item->stats_next_id = 1;
 		item->desc = &desc->item_desc[item_idx];
 
 		for (i = 0; i <= item->last_offs; i++) {
@@ -199,16 +199,16 @@
  */
 void osmo_stat_item_set(struct osmo_stat_item *item, int32_t value)
 {
+	int32_t id = item->values[item->last_offs].id + 1;
+	if (id == OSMO_STAT_ITEM_NOVALUE_ID)
+		id++;
+
 	item->last_offs += 1;
 	if (item->last_offs >= item->desc->num_values)
 		item->last_offs = 0;
 
-	global_value_id += 1;
-	if (global_value_id == OSMO_STAT_ITEM_NOVALUE_ID)
-		global_value_id += 1;
-
 	item->values[item->last_offs].value = value;
-	item->values[item->last_offs].id    = global_value_id;
+	item->values[item->last_offs].id    = id;
 }
 
 /*! Retrieve the next value from the osmo_stat_item object.
@@ -276,10 +276,8 @@
 /*! Skip all values of all items and update \a next_id accordingly */
 int osmo_stat_item_discard_all(int32_t *next_id)
 {
-	int discarded = global_value_id + 1 - *next_id;
-	*next_id = global_value_id + 1;
-
-	return discarded;
+	LOGP(DLSTATS, LOGL_DEBUG, "osmo_stat_item_discard_all is deprecated");
+	return 0;
 }
 
 /*! Initialize the stat item module. Call this once from your program.
@@ -382,7 +380,7 @@
 	unsigned int i;
 
 	item->last_offs = item->desc->num_values - 1;
-	item->last_value_index = -1;
+	item->stats_next_id = 1;
 
 	for (i = 0; i <= item->last_offs; i++) {
 		item->values[i].value = item->desc->default_value;
diff --git a/src/stats.c b/src/stats.c
index 88b733f..91cf839 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -107,7 +107,6 @@
 static LLIST_HEAD(osmo_stats_reporter_list);
 static void *osmo_stats_ctx = NULL;
 static int is_initialised = 0;
-static int32_t current_stat_item_next_id = 0;
 
 static struct osmo_stats_config s_stats_config = {
 	.interval = STATS_DEFAULT_INTERVAL,
@@ -236,12 +235,27 @@
 	talloc_free(srep);
 }
 
+static int osmo_stats_discard_item(struct osmo_stat_item_group *statg, struct osmo_stat_item *item, void *sctx_)
+{
+	return osmo_stat_item_discard(item, &item->stats_next_id);
+}
+
+static int osmo_stats_discard_group(struct osmo_stat_item_group *statg, void *sctx_)
+{
+	return osmo_stat_item_for_each_item(statg, &osmo_stats_discard_item, NULL);
+}
+
+static int osmo_stats_discard_all()
+{
+	return osmo_stat_item_for_each_group(&osmo_stats_discard_group, NULL);
+}
+
 /*! Initilize the stats reporting module; call this once in your program
  *  \param[in] ctx Talloc context from which stats related memory is allocated */
 void osmo_stats_init(void *ctx)
 {
 	osmo_stats_ctx = ctx;
-	osmo_stat_item_discard_all(&current_stat_item_next_id);
+	osmo_stats_discard_all();
 
 	is_initialised = 1;
 	start_timer();
@@ -694,18 +708,17 @@
 	struct osmo_stat_item_group *statg, struct osmo_stat_item *item, void *sctx_)
 {
 	struct osmo_stats_reporter *srep;
-	int32_t next_id = current_stat_item_next_id;
 	int32_t value;
 	int have_value;
 
-	have_value = osmo_stat_item_get_next(item, &next_id, &value) > 0;
+	have_value = osmo_stat_item_get_next(item, &item->stats_next_id, &value) > 0;
 	if (!have_value) {
 		/* Send the last value in case a flush is requested */
 		value = osmo_stat_item_get_last(item);
 	} else {
 		int32_t next_val;
 		/* If we have multiple values only send the max */
-		while (osmo_stat_item_get_next(item, &next_id, &next_val) > 0)
+		while (osmo_stat_item_get_next(item, &item->stats_next_id, &next_val) > 0)
 			value = OSMO_MAX(value, next_val);
 	}
 
@@ -798,7 +811,7 @@
 	osmo_stat_item_for_each_group(osmo_stat_item_group_handler, NULL);
 
 	/* global actions */
-	osmo_stat_item_discard_all(&current_stat_item_next_id);
+	osmo_stats_discard_all();
 	flush_all_reporters();
 	TRACE(LIBOSMOCORE_STATS_DONE());
 
diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c
index e5f7f20..b6b4be9 100644
--- a/tests/stats/stats_test.c
+++ b/tests/stats/stats_test.c
@@ -234,21 +234,6 @@
 	rc = osmo_stat_item_get_next(statg->items[TEST_A_ITEM], &next_id_a, &value);
 	OSMO_ASSERT(rc == 0);
 
-	/* Test Discard (all items) */
-	osmo_stat_item_set(statg->items[TEST_A_ITEM], 99);
-	osmo_stat_item_set(statg->items[TEST_A_ITEM], 100);
-	osmo_stat_item_set(statg->items[TEST_A_ITEM], 101);
-	osmo_stat_item_set(statg->items[TEST_B_ITEM], 99);
-	osmo_stat_item_set(statg->items[TEST_B_ITEM], 100);
-
-	rc = osmo_stat_item_discard_all(&next_id_a);
-	rc = osmo_stat_item_discard_all(&next_id_b);
-
-	rc = osmo_stat_item_get_next(statg->items[TEST_A_ITEM], &next_id_a, &value);
-	OSMO_ASSERT(rc == 0);
-	rc = osmo_stat_item_get_next(statg->items[TEST_B_ITEM], &next_id_b, &value);
-	OSMO_ASSERT(rc == 0);
-
 	osmo_stat_item_group_free(statg);
 
 	sgrp2 = osmo_stat_item_get_group_by_name_idx("test.one", 0);

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/23508
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ie65dcdf52c8fc3d916e20d7f0455f6223be6b64f
Gerrit-Change-Number: 23508
Gerrit-PatchSet: 1
Gerrit-Owner: osmith <osmith at sysmocom.de>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210326/e340f781/attachment.htm>


More information about the gerrit-log mailing list