pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-upf/+/39452?usp=email )
Change subject: Introduce hashtable to lookup chain_id ......................................................................
Introduce hashtable to lookup chain_id
This lookup was taking ages specially when UPF already managed thousands of sessions.
Related: SYS#6398 Change-Id: I7df8fd945eedbda98bd08e9fb2f382e0f55c2983 --- M include/osmocom/upf/up_gtp_action.h M include/osmocom/upf/upf.h M include/osmocom/upf/upf_nft.h M src/osmo-upf/up_gtp_action.c M src/osmo-upf/up_session.c M src/osmo-upf/upf.c M src/osmo-upf/upf_nft.c 7 files changed, 74 insertions(+), 88 deletions(-)
Approvals: Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved laforge: Looks good to me, but someone else must approve
diff --git a/include/osmocom/upf/up_gtp_action.h b/include/osmocom/upf/up_gtp_action.h index c47c35c..9bcd839 100644 --- a/include/osmocom/upf/up_gtp_action.h +++ b/include/osmocom/upf/up_gtp_action.h @@ -64,6 +64,9 @@ void *handle; };
+struct up_gtp_action *up_gtp_action_alloc(void *ctx, struct up_session *session, enum up_gtp_action_kind kind, struct llist_head *dst); +void up_gtp_action_free(struct up_gtp_action *a); + int up_gtp_action_cmp(const struct up_gtp_action *a, const struct up_gtp_action *b);
int up_gtp_action_enable(struct up_gtp_action *a); diff --git a/include/osmocom/upf/upf.h b/include/osmocom/upf/upf.h index 0e5406f..1e5d702 100644 --- a/include/osmocom/upf/upf.h +++ b/include/osmocom/upf/upf.h @@ -115,6 +115,8 @@ int priority_pre; int priority_post; uint32_t next_chain_id_state; + /* hashtable of (struct upf_nft_tun)->node_by_chain_id: */ + DECLARE_HASHTABLE(nft_tun_by_chain_id, 10); } tunmap;
struct { diff --git a/include/osmocom/upf/upf_nft.h b/include/osmocom/upf/upf_nft.h index be2ec0d..a194faf 100644 --- a/include/osmocom/upf/upf_nft.h +++ b/include/osmocom/upf/upf_nft.h @@ -24,10 +24,12 @@ #pragma once
#include <stdint.h> +#include <osmocom/core/hashtable.h>
#include <osmocom/upf/upf_tun.h>
struct upf_nft_tun { + struct hlist_node node_by_chain_id; /* item in g_upf->tunmap.nft_tun_by_chain_id */ struct upf_tun tun; uint32_t chain_id; }; diff --git a/src/osmo-upf/up_gtp_action.c b/src/osmo-upf/up_gtp_action.c index 28dd078..a3c6b68 100644 --- a/src/osmo-upf/up_gtp_action.c +++ b/src/osmo-upf/up_gtp_action.c @@ -198,3 +198,33 @@ { OSMO_NAME_C_IMPL(ctx, 128, "ERROR", up_gtp_action_to_str_buf, a) } + +struct up_gtp_action *up_gtp_action_alloc(void *ctx, struct up_session *session, enum up_gtp_action_kind kind, struct llist_head *dst) +{ + struct up_gtp_action *a = talloc_zero(ctx, struct up_gtp_action); + OSMO_ASSERT(a); + a->session = session; + a->kind = kind; + + if (kind == UP_GTP_U_TUNMAP) { + INIT_HLIST_NODE(&a->tunmap.access.node_by_chain_id); + INIT_HLIST_NODE(&a->tunmap.core.node_by_chain_id); + } + llist_add_tail(&a->entry, dst); + return a; +} + +void up_gtp_action_free(struct up_gtp_action *a) +{ + if (!a) + return; + up_gtp_action_disable(a); + llist_del(&a->entry); + if (a->kind == UP_GTP_U_TUNMAP) { + if (!hlist_unhashed(&a->tunmap.access.node_by_chain_id)) + hash_del(&a->tunmap.access.node_by_chain_id); + if (!hlist_unhashed(&a->tunmap.core.node_by_chain_id)) + hash_del(&a->tunmap.core.node_by_chain_id); + } + talloc_free(a); +} diff --git a/src/osmo-upf/up_session.c b/src/osmo-upf/up_session.c index 359d116..97c9367 100644 --- a/src/osmo-upf/up_session.c +++ b/src/osmo-upf/up_session.c @@ -824,9 +824,7 @@
/* Shut down all active GTP rules */ while ((a = llist_first_entry_or_null(&session->active_gtp_actions, struct up_gtp_action, entry))) { - up_gtp_action_disable(a); - llist_del(&a->entry); - talloc_free(a); + up_gtp_action_free(a); } }
@@ -1202,31 +1200,14 @@ talloc_free(rpdr->inactive_reason); rpdr->inactive_reason = NULL;
- a = talloc(ctx, struct up_gtp_action); - OSMO_ASSERT(a); - *a = (struct up_gtp_action){ - .session = session, - .pdr_access = pdr->desc.pdr_id, - .pdr_core = rpdr->desc.pdr_id, - .kind = UP_GTP_U_TUNEND, - .tunend = { - .access = { - .local = { - .addr = pdr->local_f_teid->fixed.ip_addr.v4, - .teid = pdr->local_f_teid->fixed.teid, - }, - .remote = { - .addr = rfar_forw->outer_header_creation.ip_addr.v4, - .teid = rfar_forw->outer_header_creation.teid, - }, - }, - .core = { - .ue_local_addr = rpdr->desc.pdi.ue_ip_address.ip_addr.v4, - }, - }, - }; - - llist_add_tail(&a->entry, dst); + a = up_gtp_action_alloc(ctx, session, UP_GTP_U_TUNEND, dst); + a->pdr_access = pdr->desc.pdr_id; + a->pdr_core = rpdr->desc.pdr_id; + a->tunend.access.local.addr = pdr->local_f_teid->fixed.ip_addr.v4; + a->tunend.access.local.teid = pdr->local_f_teid->fixed.teid; + a->tunend.access.remote.addr = rfar_forw->outer_header_creation.ip_addr.v4; + a->tunend.access.remote.teid = rfar_forw->outer_header_creation.teid; + a->tunend.core.ue_local_addr = rpdr->desc.pdi.ue_ip_address.ip_addr.v4; }
/* A GTP tunnel on Access side, mapping to another GTP tunnel on Core side and vice versa. @@ -1316,38 +1297,17 @@ talloc_free(rpdr->inactive_reason); rpdr->inactive_reason = NULL;
- a = talloc(ctx, struct up_gtp_action); - OSMO_ASSERT(a); - *a = (struct up_gtp_action){ - .session = session, - .pdr_access = pdr->desc.pdr_id, - .pdr_core = rpdr->desc.pdr_id, - .kind = UP_GTP_U_TUNMAP, - .tunmap = { - .access.tun = { - .local = { - .addr = pdr->local_f_teid->fixed.ip_addr.v4, - .teid = pdr->local_f_teid->fixed.teid, - }, - .remote = { - .addr = rfar_forw->outer_header_creation.ip_addr.v4, - .teid = rfar_forw->outer_header_creation.teid, - }, - }, - .core.tun = { - .local = { - .addr = rpdr->local_f_teid->fixed.ip_addr.v4, - .teid = rpdr->local_f_teid->fixed.teid, - }, - .remote = { - .addr = far_forw->outer_header_creation.ip_addr.v4, - .teid = far_forw->outer_header_creation.teid, - }, - }, - }, - }; - - llist_add_tail(&a->entry, dst); + a = up_gtp_action_alloc(ctx, session, UP_GTP_U_TUNMAP, dst); + a->pdr_access = pdr->desc.pdr_id; + a->pdr_core = rpdr->desc.pdr_id; + a->tunmap.access.tun.local.addr = pdr->local_f_teid->fixed.ip_addr.v4; + a->tunmap.access.tun.local.teid = pdr->local_f_teid->fixed.teid; + a->tunmap.access.tun.remote.addr = rfar_forw->outer_header_creation.ip_addr.v4; + a->tunmap.access.tun.remote.teid = rfar_forw->outer_header_creation.teid; + a->tunmap.core.tun.local.addr = rpdr->local_f_teid->fixed.ip_addr.v4; + a->tunmap.core.tun.local.teid = rpdr->local_f_teid->fixed.teid; + a->tunmap.core.tun.remote.addr = far_forw->outer_header_creation.ip_addr.v4; + a->tunmap.core.tun.remote.teid = far_forw->outer_header_creation.teid; }
/* Analyse all PDRs and FARs and find configurations that match either a GTP encaps/decaps or a GTP forward rule. Add to @@ -1472,9 +1432,7 @@ continue;
LOGPFSML(session->fi, LOGL_DEBUG, "disabling: %s\n", up_gtp_action_to_str_c(OTC_SELECT, a)); - up_gtp_action_disable(a); - llist_del(&a->entry); - talloc_free(a); + up_gtp_action_free(a); }
/* Set up all GTP tunnels requested in the session setup, but not active yet */ diff --git a/src/osmo-upf/upf.c b/src/osmo-upf/upf.c index c82227d..e442d1c 100644 --- a/src/osmo-upf/upf.c +++ b/src/osmo-upf/upf.c @@ -78,6 +78,7 @@ INIT_LLIST_HEAD(&g_upf->tunend.vty_cfg.devs); INIT_LLIST_HEAD(&g_upf->tunend.devs); INIT_LLIST_HEAD(&g_upf->netinst); + hash_init(g_upf->tunmap.nft_tun_by_chain_id); hash_init(g_upf->gtp.pdrs_by_local_f_teid); }
@@ -175,6 +176,17 @@ return g_upf->tunmap.next_chain_id_state; }
+static bool upf_is_chain_id_in_use(uint32_t chain_id) +{ + struct upf_nft_tun *nft_tun; + hash_for_each_possible(g_upf->tunmap.nft_tun_by_chain_id, nft_tun, node_by_chain_id, chain_id) { + if (nft_tun->chain_id != chain_id) + continue; + return true; + } + return false; +} + /* Return an unused chain_id, or 0 if none is found with sane effort. */ uint32_t upf_next_chain_id(void) { @@ -182,36 +194,14 @@
/* Make sure the new chain_id is not used anywhere */ for (sanity = 2342; sanity; sanity--) { - struct up_peer *peer; uint32_t chain_id = upf_next_chain_id_inc(); - bool taken = false;
if (!g_upf->pfcp.ep) return chain_id;
- llist_for_each_entry(peer, &g_upf->pfcp.ep->peers, entry) { - struct up_session *session; - int bkt; - hash_for_each(peer->sessions_by_up_seid, bkt, session, node_by_up_seid) { - struct up_gtp_action *a; - llist_for_each_entry(a, &session->active_gtp_actions, entry) { - if (a->kind != UP_GTP_U_TUNMAP) - continue; - if (a->tunmap.access.chain_id == chain_id - || a->tunmap.core.chain_id == chain_id) { - taken = true; - break; - } - } - if (taken) - break; - } - if (taken) - break; - } - - if (!taken) - return chain_id; + if (upf_is_chain_id_in_use(chain_id)) + continue; + return chain_id; }
/* finding a chain_id became insane, return invalid = 0 */ diff --git a/src/osmo-upf/upf_nft.c b/src/osmo-upf/upf_nft.c index bc0ee36..7e5b80f 100644 --- a/src/osmo-upf/upf_nft.c +++ b/src/osmo-upf/upf_nft.c @@ -489,6 +489,7 @@ tun->chain_id = upf_next_chain_id(); if (!tun->chain_id) return -ENOSPC; + hash_add(g_upf->tunmap.nft_tun_by_chain_id, &tun->node_by_chain_id, tun->chain_id); return 0; }
-
pespin