<p>laforge <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/23508">View Change</a></p><div style="white-space:pre-wrap">Approvals:
laforge: Looks good to me, approved
daniel: Looks good to me, but someone else must approve
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">stat_item: make value ids item specific<br><br>Fix counting of values missed because of FIFO overflow in<br>osmo_stat_item_get_next(), by assigning a new item value id effectively<br>as item->value[n + 1].id = item->value[n].id + 1, instead of increasing<br>a global_value_id that is shared between all items and groups. With<br>global_value_id, the count of values missed was wrong for one item, as<br>soon as a new value was added to another item.<br><br>This partially reverts b27b352e ("stats: Use a global index for stat<br>item values") from 2015, right after stats was added to libosmocore. It<br>was supposed to make multiple readers (reporters) possible, which could<br>read independently from stat_item (and later added comments explain it<br>like that). But this remained unused, stats has implemented multiple<br>reporters by reading all stat_items once and sending the same data to<br>all enabled reporters. The patch caused last_value_index in struct<br>osmo_stat_item to always remain at -1.<br><br>Replace this unused last_value_index with stats_next_id, so stats can<br>store the item-specific next_id in the struct again. It appears that<br>stats is the only direct user of osmo_stat_item, but if there are<br>others, they can bring their own item-specific next_id: functions in<br>stat_item.c still accept a next_id argument.<br><br>Related: OS#5088<br>Change-Id: Ie65dcdf52c8fc3d916e20d7f0455f6223be6b64f<br>---<br>M include/osmocom/core/stat_item.h<br>M src/stat_item.c<br>M src/stats.c<br>M tests/stats/stats_test.c<br>4 files changed, 44 insertions(+), 44 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/include/osmocom/core/stat_item.h b/include/osmocom/core/stat_item.h</span><br><span>index 4a19465..29e35ef 100644</span><br><span>--- a/include/osmocom/core/stat_item.h</span><br><span>+++ b/include/osmocom/core/stat_item.h</span><br><span>@@ -6,6 +6,7 @@</span><br><span> </span><br><span> #include <stdint.h></span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#include <osmocom/core/defs.h></span><br><span> #include <osmocom/core/linuxlist.h></span><br><span> </span><br><span> struct osmo_stat_item_desc;</span><br><span>@@ -23,9 +24,11 @@</span><br><span> struct osmo_stat_item {</span><br><span> /*! back-reference to the item description */</span><br><span> const struct osmo_stat_item_desc *desc;</span><br><span style="color: hsl(0, 100%, 40%);">- /*! the index of the freshest value */</span><br><span style="color: hsl(0, 100%, 40%);">- int32_t last_value_index;</span><br><span style="color: hsl(0, 100%, 40%);">- /*! offset to the freshest value in the value FIFO */</span><br><span style="color: hsl(120, 100%, 40%);">+ /* internal use by stats API (stats.c): the id of the next value to</span><br><span style="color: hsl(120, 100%, 40%);">+ * be read from the FIFO. If accessing osmo_stat_item directly, without</span><br><span style="color: hsl(120, 100%, 40%);">+ * the stats API, store this value elsewhere. */</span><br><span style="color: hsl(120, 100%, 40%);">+ int32_t stats_next_id;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*! the index of the last value written to the FIFO */</span><br><span> int16_t last_offs;</span><br><span> /*! value FIFO */</span><br><span> struct osmo_stat_item_value values[0];</span><br><span>@@ -98,7 +101,8 @@</span><br><span> </span><br><span> int osmo_stat_item_discard(const struct osmo_stat_item *item, int32_t *next_id);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-int osmo_stat_item_discard_all(int32_t *next_id);</span><br><span style="color: hsl(120, 100%, 40%);">+int osmo_stat_item_discard_all(int32_t *next_id)</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_DEPRECATED("Use osmo_stat_item_discard with item-specific next_id instead");</span><br><span> </span><br><span> typedef int (*osmo_stat_item_handler_t)(</span><br><span> struct osmo_stat_item_group *, struct osmo_stat_item *, void *);</span><br><span>diff --git a/src/stat_item.c b/src/stat_item.c</span><br><span>index d4f3b72..2d96444 100644</span><br><span>--- a/src/stat_item.c</span><br><span>+++ b/src/stat_item.c</span><br><span>@@ -37,14 +37,15 @@</span><br><span> *</span><br><span> * The only supported value type is an int32_t.</span><br><span> *</span><br><span style="color: hsl(0, 100%, 40%);">- * Getting values from the osmo_stat_item does not modify its state to</span><br><span style="color: hsl(0, 100%, 40%);">- * allow for multiple independent back-ends retrieving values (e.g. VTY</span><br><span style="color: hsl(0, 100%, 40%);">- * and statd).</span><br><span style="color: hsl(120, 100%, 40%);">+ * Getting values from osmo_stat_item is usually done at a high level</span><br><span style="color: hsl(120, 100%, 40%);">+ * through the stats API (stats.c). It uses item->stats_next_id to</span><br><span style="color: hsl(120, 100%, 40%);">+ * store what has been sent to all enabled reporters. It is also</span><br><span style="color: hsl(120, 100%, 40%);">+ * possible to read from osmo_stat_item directly, without modifying</span><br><span style="color: hsl(120, 100%, 40%);">+ * its state, by storing next_id outside of osmo_stat_item.</span><br><span> *</span><br><span> * Each value stored in the FIFO of an osmo_stat_item has an associated</span><br><span style="color: hsl(0, 100%, 40%);">- * value_id. The value_id is derived from an application-wide globally</span><br><span style="color: hsl(0, 100%, 40%);">- * incrementing counter, so (until the counter wraps) more recent</span><br><span style="color: hsl(0, 100%, 40%);">- * values will have higher values.</span><br><span style="color: hsl(120, 100%, 40%);">+ * value_id. The value_id is increased with each value, so (until the</span><br><span style="color: hsl(120, 100%, 40%);">+ * counter wraps) more recent values will have higher values.</span><br><span> *</span><br><span> * When a new value is set, the oldest value in the FIFO gets silently</span><br><span> * overwritten. Lost values are skipped when getting values from the</span><br><span>@@ -74,14 +75,13 @@</span><br><span> </span><br><span> #include <osmocom/core/utils.h></span><br><span> #include <osmocom/core/linuxlist.h></span><br><span style="color: hsl(120, 100%, 40%);">+#include <osmocom/core/logging.h></span><br><span> #include <osmocom/core/talloc.h></span><br><span> #include <osmocom/core/timer.h></span><br><span> #include <osmocom/core/stat_item.h></span><br><span> </span><br><span> /*! global list of stat_item groups */</span><br><span> static LLIST_HEAD(osmo_stat_item_groups);</span><br><span style="color: hsl(0, 100%, 40%);">-/*! counter for assigning globally unique value identifiers */</span><br><span style="color: hsl(0, 100%, 40%);">-static int32_t global_value_id = 0;</span><br><span> </span><br><span> /*! talloc context from which we allocate */</span><br><span> static void *tall_stat_item_ctx;</span><br><span>@@ -146,7 +146,7 @@</span><br><span> </span><br><span> group->items[item_idx] = item;</span><br><span> item->last_offs = desc->item_desc[item_idx].num_values - 1;</span><br><span style="color: hsl(0, 100%, 40%);">- item->last_value_index = -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ item->stats_next_id = 1;</span><br><span> item->desc = &desc->item_desc[item_idx];</span><br><span> </span><br><span> for (i = 0; i <= item->last_offs; i++) {</span><br><span>@@ -199,16 +199,16 @@</span><br><span> */</span><br><span> void osmo_stat_item_set(struct osmo_stat_item *item, int32_t value)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ int32_t id = item->values[item->last_offs].id + 1;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (id == OSMO_STAT_ITEM_NOVALUE_ID)</span><br><span style="color: hsl(120, 100%, 40%);">+ id++;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> item->last_offs += 1;</span><br><span> if (item->last_offs >= item->desc->num_values)</span><br><span> item->last_offs = 0;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- global_value_id += 1;</span><br><span style="color: hsl(0, 100%, 40%);">- if (global_value_id == OSMO_STAT_ITEM_NOVALUE_ID)</span><br><span style="color: hsl(0, 100%, 40%);">- global_value_id += 1;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> item->values[item->last_offs].value = value;</span><br><span style="color: hsl(0, 100%, 40%);">- item->values[item->last_offs].id = global_value_id;</span><br><span style="color: hsl(120, 100%, 40%);">+ item->values[item->last_offs].id = id;</span><br><span> }</span><br><span> </span><br><span> /*! Retrieve the next value from the osmo_stat_item object.</span><br><span>@@ -276,10 +276,8 @@</span><br><span> /*! Skip all values of all items and update \a next_id accordingly */</span><br><span> int osmo_stat_item_discard_all(int32_t *next_id)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- int discarded = global_value_id + 1 - *next_id;</span><br><span style="color: hsl(0, 100%, 40%);">- *next_id = global_value_id + 1;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- return discarded;</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DLSTATS, LOGL_ERROR, "osmo_stat_item_discard_all is deprecated (no-op)\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span> }</span><br><span> </span><br><span> /*! Initialize the stat item module. Call this once from your program.</span><br><span>@@ -382,7 +380,7 @@</span><br><span> unsigned int i;</span><br><span> </span><br><span> item->last_offs = item->desc->num_values - 1;</span><br><span style="color: hsl(0, 100%, 40%);">- item->last_value_index = -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ item->stats_next_id = 1;</span><br><span> </span><br><span> for (i = 0; i <= item->last_offs; i++) {</span><br><span> item->values[i].value = item->desc->default_value;</span><br><span>diff --git a/src/stats.c b/src/stats.c</span><br><span>index 88b733f..91cf839 100644</span><br><span>--- a/src/stats.c</span><br><span>+++ b/src/stats.c</span><br><span>@@ -107,7 +107,6 @@</span><br><span> static LLIST_HEAD(osmo_stats_reporter_list);</span><br><span> static void *osmo_stats_ctx = NULL;</span><br><span> static int is_initialised = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-static int32_t current_stat_item_next_id = 0;</span><br><span> </span><br><span> static struct osmo_stats_config s_stats_config = {</span><br><span> .interval = STATS_DEFAULT_INTERVAL,</span><br><span>@@ -236,12 +235,27 @@</span><br><span> talloc_free(srep);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static int osmo_stats_discard_item(struct osmo_stat_item_group *statg, struct osmo_stat_item *item, void *sctx_)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ return osmo_stat_item_discard(item, &item->stats_next_id);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+static int osmo_stats_discard_group(struct osmo_stat_item_group *statg, void *sctx_)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ return osmo_stat_item_for_each_item(statg, &osmo_stats_discard_item, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+static int osmo_stats_discard_all()</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ return osmo_stat_item_for_each_group(&osmo_stats_discard_group, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! Initilize the stats reporting module; call this once in your program</span><br><span> * \param[in] ctx Talloc context from which stats related memory is allocated */</span><br><span> void osmo_stats_init(void *ctx)</span><br><span> {</span><br><span> osmo_stats_ctx = ctx;</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_discard_all(¤t_stat_item_next_id);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_stats_discard_all();</span><br><span> </span><br><span> is_initialised = 1;</span><br><span> start_timer();</span><br><span>@@ -694,18 +708,17 @@</span><br><span> struct osmo_stat_item_group *statg, struct osmo_stat_item *item, void *sctx_)</span><br><span> {</span><br><span> struct osmo_stats_reporter *srep;</span><br><span style="color: hsl(0, 100%, 40%);">- int32_t next_id = current_stat_item_next_id;</span><br><span> int32_t value;</span><br><span> int have_value;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- have_value = osmo_stat_item_get_next(item, &next_id, &value) > 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ have_value = osmo_stat_item_get_next(item, &item->stats_next_id, &value) > 0;</span><br><span> if (!have_value) {</span><br><span> /* Send the last value in case a flush is requested */</span><br><span> value = osmo_stat_item_get_last(item);</span><br><span> } else {</span><br><span> int32_t next_val;</span><br><span> /* If we have multiple values only send the max */</span><br><span style="color: hsl(0, 100%, 40%);">- while (osmo_stat_item_get_next(item, &next_id, &next_val) > 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ while (osmo_stat_item_get_next(item, &item->stats_next_id, &next_val) > 0)</span><br><span> value = OSMO_MAX(value, next_val);</span><br><span> }</span><br><span> </span><br><span>@@ -798,7 +811,7 @@</span><br><span> osmo_stat_item_for_each_group(osmo_stat_item_group_handler, NULL);</span><br><span> </span><br><span> /* global actions */</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_discard_all(¤t_stat_item_next_id);</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_stats_discard_all();</span><br><span> flush_all_reporters();</span><br><span> TRACE(LIBOSMOCORE_STATS_DONE());</span><br><span> </span><br><span>diff --git a/tests/stats/stats_test.c b/tests/stats/stats_test.c</span><br><span>index e5f7f20..b6b4be9 100644</span><br><span>--- a/tests/stats/stats_test.c</span><br><span>+++ b/tests/stats/stats_test.c</span><br><span>@@ -234,21 +234,6 @@</span><br><span> rc = osmo_stat_item_get_next(statg->items[TEST_A_ITEM], &next_id_a, &value);</span><br><span> OSMO_ASSERT(rc == 0);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* Test Discard (all items) */</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_set(statg->items[TEST_A_ITEM], 99);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_set(statg->items[TEST_A_ITEM], 100);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_set(statg->items[TEST_A_ITEM], 101);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_set(statg->items[TEST_B_ITEM], 99);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_stat_item_set(statg->items[TEST_B_ITEM], 100);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- rc = osmo_stat_item_discard_all(&next_id_a);</span><br><span style="color: hsl(0, 100%, 40%);">- rc = osmo_stat_item_discard_all(&next_id_b);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">- rc = osmo_stat_item_get_next(statg->items[TEST_A_ITEM], &next_id_a, &value);</span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_ASSERT(rc == 0);</span><br><span style="color: hsl(0, 100%, 40%);">- rc = osmo_stat_item_get_next(statg->items[TEST_B_ITEM], &next_id_b, &value);</span><br><span style="color: hsl(0, 100%, 40%);">- OSMO_ASSERT(rc == 0);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> osmo_stat_item_group_free(statg);</span><br><span> </span><br><span> sgrp2 = osmo_stat_item_get_group_by_name_idx("test.one", 0);</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/23508">change 23508</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/23508"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ie65dcdf52c8fc3d916e20d7f0455f6223be6b64f </div>
<div style="display:none"> Gerrit-Change-Number: 23508 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>