neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/36540?usp=email )
Change subject: nft_kpi: retrieve counters in a separate thread ......................................................................
nft_kpi: retrieve counters in a separate thread
Introduce an NFT thread which does: - periodically run nftables command to read all counters - parse the response - update rate_ctr values.
The main thread still runs the rule addition / removal when a HNB registers or deregisters. See the comment added in nft_kpi.c, starting with "A more scalable solution...".
This patch requires the new osmo_stats_report_lock(), see 'Depends'.
Related: SYS#6773 Depends: libosmocore Ib335bea7d2a440ca284e6c439066f96456bf2c2d Change-Id: I9dc54e6bc94c553f45adfa71ae8ad70be4afbc8f --- M src/osmo-hnbgw/hnbgw.c M src/osmo-hnbgw/nft_kpi.c M src/osmo-hnbgw/osmo_hnbgw_main.c M src/osmo-hnbgw/tdefs.c 4 files changed, 171 insertions(+), 34 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/40/36540/1
diff --git a/src/osmo-hnbgw/hnbgw.c b/src/osmo-hnbgw/hnbgw.c index 6340d10..fa516ea 100644 --- a/src/osmo-hnbgw/hnbgw.c +++ b/src/osmo-hnbgw/hnbgw.c @@ -513,6 +513,12 @@ if (!hnbp) return NULL;
+ /* nft_kpi.c updates hnb stats, and it locks osmo_stats_report_lock() while it does so. This here should run + * in the same thread as stats reporting, so there should be no conflict with stats. But to avoid a hnb in flux + * while nft_kpi.c looks up hnb, also obtain the osmo_stats_report_lock() while manipulating hnb records. */ + osmo_stats_report_lock(true); + /* { */ + hnbp->id = *id; hnbp->id_str = talloc_strdup(hnbp, umts_cell_id_name(id)); hnbp->ctrs = rate_ctr_group_alloc(hnbp, &hnb_ctrg_desc, 0); @@ -525,14 +531,21 @@ osmo_stat_item_group_set_name(hnbp->statg, hnbp->id_str);
llist_add(&hnbp->list, &g_hnbgw->hnb_persistent_list); + /* success */ + goto out_unlock;
- return hnbp; - + /* failure */ out_free_ctrs: rate_ctr_group_free(hnbp->ctrs); out_free: talloc_free(hnbp); - return NULL; + hnbp = NULL; + + /* for both success and failure: */ +out_unlock: + /* } */ + osmo_stats_report_lock(false); + return hnbp; }
struct hnb_persistent *hnb_persistent_find_by_id(const struct umts_cell_id *id) diff --git a/src/osmo-hnbgw/nft_kpi.c b/src/osmo-hnbgw/nft_kpi.c index f585415..039c891 100644 --- a/src/osmo-hnbgw/nft_kpi.c +++ b/src/osmo-hnbgw/nft_kpi.c @@ -15,7 +15,11 @@ * GNU Affero General Public License for more details. */
+#include <pthread.h> +#include <unistd.h> + #include <osmocom/core/logging.h> +#include <osmocom/core/stats.h> #include <osmocom/hnbgw/hnbgw.h>
#include "config.h" @@ -46,13 +50,68 @@ #include <osmocom/hnbgw/nft_kpi.h> #include <osmocom/hnbgw/tdefs.h>
+/* Threading and locks in this implementation: + * + * - osmo_stats_report_lock() held while updating rate_ctr from nft results. + * - g_nft_kpi_state.lock held while running an nftables command buffer. + * + * contrived example: + * + * Main Thread + * | + * osmo_stats_report_use_lock(true) + * | + * nft_kpi_init() + * create nft ctx, create table + * | + * +---------------------------> NFT thread + * | | + * | sleep(X34) + * | | + * | LOCK(g_nft_kpi_state.lock) + * | | + * osmo_stats_report() query all nft counters + * LOCK(osmo_stats_report_lock) | + * | LOCK(osmo_stats_report_lock) + * collect stats : wait because libosmocore stats reporting is busy + * | : + * UNLOCK(osmo_stats_report_lock) LOCK------| + * send out stats for all hnbp: rate_ctr_add2() + * | | + * | UNLOCK(osmo_stats_report_lock) + * | | + * | UNLOCK(g_nft_kpi_state.lock) + * | | + * hnbgw_rx_hnb_register_req() sleep(X34) + * hnb_nft_kpi_start() | + * LOCK(g_nft_kpi_state.lock) ... + * | + * nftables: add new rule + * | + * UNLOCK(g_nft_kpi_state.lock) + * | + * ... + * + * So the NFT thread only retrieves counters. The main thread adds and removes NFT rules for counters. It is possible + * that a HNBAP HNB Register or HNB De-Register occurrs while the NFT thread holds the g_nft_kpi_state.lock, so that the + * main thread blocks until the NFT thread is done reading counters. Note, this happens only for HNB attach or detach. + * + * A more scalable solution is to move all NFT interaction to the thread. Instead of submitting rules from the main + * thread, we could submit instructions to an inter-thread queue that the NFT thread works off. This would add + * considerable complexity -- for now we accept the possible but rarely occurring short delay for HNB de/registration. + */ + struct nft_kpi_state { + /* lock this while modifying g_nft_kpi_state */ + pthread_mutex_t lock; + struct { struct nft_ctx *nft_ctx; char *table_name; bool table_created; } nft; - struct osmo_timer_list period; + + pthread_t thread; };
static struct nft_kpi_state g_nft_kpi_state = {}; @@ -98,15 +157,22 @@ return 0; }
-static void nft_kpi_period_cb(void *data); +static void nft_kpi_period_cb(void);
-static void nft_kpi_period_schedule(void) +void *nft_kpi_thread(void *arg) { - unsigned long period = osmo_tdef_get(hnbgw_T_defs, -34, OSMO_TDEF_S, 10); - if (period < 1) - period = 1; - osmo_timer_setup(&g_nft_kpi_state.period, nft_kpi_period_cb, NULL); - osmo_timer_schedule(&g_nft_kpi_state.period, period, 0); + OSMO_ASSERT(osmo_ctx_init(__func__) == 0); + while (1) { + /* Let's just hope that the unsigned long in the hnbgw_T_defs is not modified non-atomically while + * reading the timeout value. */ + unsigned long period = osmo_tdef_get(hnbgw_T_defs, -34, OSMO_TDEF_US, 1000000); + if (period < 1) + period = 1; + usleep(period); + + nft_kpi_period_cb(); + } + return NULL; }
int nft_kpi_init(const char *table_name) @@ -145,7 +211,11 @@ return -EIO;
s->nft.table_created = true; - nft_kpi_period_schedule(); + + /* Up to here, it was fine to dispatch nft without locking, because this is the initialization from the main + * thread. From now on, whoever wants to use the g_nft_ctx must lock this mutex first. */ + pthread_mutex_init(&s->lock, NULL); + pthread_create(&s->thread, NULL, nft_kpi_thread, NULL); return 0; }
@@ -163,14 +233,19 @@ return -EINVAL; }
+ /* Manipulating nft state, obtain lock */ + pthread_mutex_lock(&s->lock); + /* { */ + if (!osmo_sockaddr_str_cmp(gtpu_remote, &hnbp->nft_kpi.addr_remote)) { /* The remote address is unchanged, no need to update the nft probe */ - return 0; + rc = 0; + goto unlock_return; }
/* When there is no table created, it means nft is disabled. Do not attempt to set up counters. */ if (!s->nft.table_created) - return 0; + goto unlock_return;
/* The remote address has changed. Cancel previous probe, if any, and start a new one. */ if (osmo_sockaddr_str_is_nonzero(&hnbp->nft_kpi.addr_remote)) @@ -196,6 +271,10 @@ /* There was an error running the rule, clear addr_remote to indicate that no rule exists. */ hnbp->nft_kpi.addr_remote = (struct osmo_sockaddr_str){}; } + +unlock_return: + /* } */ + pthread_mutex_unlock(&s->lock); return rc; }
@@ -206,15 +285,19 @@ { struct nft_kpi_state *s = &g_nft_kpi_state; char *cmd; + int rc = 0;
/* When there is no table created, neither can there be any rules to be deleted. * The rules get removed, but the table remains present for as long as osmo-hnbgw runs. */ if (!s->nft.table_created) return 0;
+ pthread_mutex_lock(&s->lock); + /* { */ + /* presence of addr_remote indicates whether an nft rule has been submitted and still needs to be removed */ if (!osmo_sockaddr_str_is_nonzero(&hnbp->nft_kpi.addr_remote)) - return 0; + goto unlock_return;
if (!hnbp->nft_kpi.last.ul.handle_present || !hnbp->nft_kpi.last.dl.handle_present) { @@ -235,7 +318,12 @@ hnbp->nft_kpi.last.ul.handle, s->nft.table_name, hnbp->nft_kpi.last.dl.handle); - return nft_run_now(cmd); + rc = nft_run_now(cmd); + +unlock_return: + /* } */ + pthread_mutex_unlock(&s->lock); + return rc; }
static void update_ctr(struct rate_ctr_group *cg, int cgidx, uint64_t *last_val, uint64_t new_val) @@ -383,6 +471,7 @@ LOGP(DNFT, LOGL_DEBUG, "read %d counters from nft table %s\n", count, s->nft.table_name); }
+/* The caller must hold the g_nft_kpi_state.lock! */ static void nft_kpi_read_counters(void) { int rc; @@ -399,6 +488,12 @@ OSMO_STRBUF_PRINTF(sb, "list table inet %s", s->nft.table_name); OSMO_ASSERT(sb.chars_needed < sizeof(cmd));
+ size_t l = strlen(cmd); + LOGP(DNFT, LOGL_DEBUG, "running nft request, %zu chars: "%s%s"\n", + l, + osmo_escape_cstr_c(OTC_SELECT, cmd, OSMO_MIN(logmax, l)), + l > logmax ? "..." : ""); + rc = nft_ctx_buffer_output(nft); if (rc) { LOGP(DNFT, LOGL_ERROR, "error: nft_ctx_buffer_output() returned failure: rc=%d cmd=%s\n", @@ -413,29 +508,29 @@ }
output = nft_ctx_get_output_buffer(nft); - if (log_check_level(DNFT, LOGL_DEBUG)) { - size_t l = strlen(cmd); - LOGP(DNFT, LOGL_DEBUG, "ran nft request, %zu chars: "%s%s"\n", - l, - osmo_escape_cstr_c(OTC_SELECT, cmd, OSMO_MIN(logmax, l)), - l > logmax ? "..." : ""); - l = strlen(output); - LOGP(DNFT, LOGL_DEBUG, "got nft response, %zu chars: "%s%s"\n", - l, - osmo_escape_cstr_c(OTC_SELECT, output, OSMO_MIN(logmax, l)), - l > logmax ? "..." : ""); - } + l = strlen(output); + LOGP(DNFT, LOGL_DEBUG, "got nft response, %zu chars: "%s%s"\n", + l, + osmo_escape_cstr_c(OTC_SELECT, output, OSMO_MIN(logmax, l)), + l > logmax ? "..." : "");
+ osmo_stats_report_lock(true); + /* { */ decode_nft_response(output); + /* } */ + osmo_stats_report_lock(false);
unbuffer_and_exit: nft_ctx_unbuffer_output(nft); }
-static void nft_kpi_period_cb(void *data) +static void nft_kpi_period_cb(void) { + pthread_mutex_lock(&g_nft_kpi_state.lock); + /* { */ nft_kpi_read_counters(); - nft_kpi_period_schedule(); + /* } */ + pthread_mutex_unlock(&g_nft_kpi_state.lock); }
#endif // ENABLE_NFTABLES diff --git a/src/osmo-hnbgw/osmo_hnbgw_main.c b/src/osmo-hnbgw/osmo_hnbgw_main.c index 1052d18..36180ef 100644 --- a/src/osmo-hnbgw/osmo_hnbgw_main.c +++ b/src/osmo-hnbgw/osmo_hnbgw_main.c @@ -328,16 +328,23 @@ /* If UPF is configured, set up PFCP socket and send Association Setup Request to UPF */ hnbgw_pfcp_init(); #endif -#if ENABLE_NFTABLES + /* If nftables is enabled, initialize the nft table now or fail startup. This is important to immediately let * the user know if cap_net_admin privileges are missing, and not only when the first hNodeB connects. */ if (g_hnbgw->config.nft_kpi.enable) { +#if ENABLE_NFTABLES if (nft_kpi_init(g_hnbgw->config.nft_kpi.table_name)) { - perror("Failed to initialize nft KPI, probably missing cap_net_admin"); + perror("ERROR: Failed to initialize nft KPI, probably missing cap_net_admin"); exit(1); } - } + /* nft_kpi.c manipulates rate_ctr state directly. Enable the mutex lock around stats reporting, so + * nft_kpi.c can make use of it. */ + osmo_stats_report_use_lock(true); +#else + fprintf(stderr, "ERROR: Cannot enable nft KPI, this binary was built without nftables support\n"); + exit(1); #endif + }
hnbgw_cnpool_start(&g_hnbgw->sccp.cnpool_iucs); hnbgw_cnpool_start(&g_hnbgw->sccp.cnpool_iups); diff --git a/src/osmo-hnbgw/tdefs.c b/src/osmo-hnbgw/tdefs.c index 3a7f0bc..cfcd4c2 100644 --- a/src/osmo-hnbgw/tdefs.c +++ b/src/osmo-hnbgw/tdefs.c @@ -35,8 +35,8 @@ {.T = 3113, .default_val = 15, .desc = "Time to keep Paging record, for CN pools with more than one link" }, {.T = 4, .default_val = 5, .desc = "Timeout to receive RANAP RESET ACKNOWLEDGE from an MSC/SGSN" }, {.T = -31, .default_val = 15, .desc = "Timeout for establishing and releasing context maps (RUA <-> SCCP)" }, + {.T = -34, .default_val = 1000, .unit = OSMO_TDEF_MS, .desc = "Period to query network traffic stats from netfilter" }, {.T = -1002, .default_val = 10, .desc = "Timeout for the HNB to respond to PS RAB Assignment Request" }, - {.T = -34, .default_val = 1, .desc = "Period to query network traffic stats from netfilter" }, { } };
-
neels