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

2 files changed, 36 insertions, 25 deletions

diff --git a/src/stat_item.c b/src/stat_item.c
index d4f3b727..2d964445 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 @@ struct osmo_stat_item_group *osmo_stat_item_group_alloc(void *ctx,
 
 		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_dec(struct osmo_stat_item *item, int32_t value)
  */
 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 @@ int osmo_stat_item_discard(const struct osmo_stat_item *item, int32_t *next_id)
 /*! 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_ERROR, "osmo_stat_item_discard_all is deprecated (no-op)\n");
+	return 0;
 }
 
 /*! Initialize the stat item module. Call this once from your program.
@@ -382,7 +380,7 @@ void osmo_stat_item_reset(struct osmo_stat_item *item)
 	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 88b733fd..91cf839b 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 @@ void osmo_stats_reporter_free(struct osmo_stats_reporter *srep)
 	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 @@ static int osmo_stat_item_handler(
 	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 @@ int osmo_stats_report()
 	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());