This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has submitted this change. ( 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(-)
Approvals:
laforge: Looks good to me, approved
daniel: Looks good to me, but someone else must approve
Jenkins Builder: Verified
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..2d96444 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_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 @@
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(¤t_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(¤t_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: 4
Gerrit-Owner: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210407/3af5287e/attachment.htm>