Change in libosmocore[master]: WIP: gprs_ns2: start use (ref) counting

lynxis lazus gerrit-no-reply at lists.osmocom.org
Mon Jul 12 15:53:24 UTC 2021


lynxis lazus has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/24923 )


Change subject: WIP: gprs_ns2: start use (ref) counting
......................................................................

WIP: gprs_ns2: start use (ref) counting

Also avoid recursive free() rounds within the ns2 code

Change-Id: I03cb2419926c639d4bd357a33ce8008c50cd3bee
---
M src/gb/gprs_ns2.c
M src/gb/gprs_ns2_internal.h
M src/gb/gprs_ns2_sns.c
M tests/gb/gprs_ns2_test.c
M tests/gb/gprs_ns2_test.ok
5 files changed, 137 insertions(+), 17 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/23/24923/1

diff --git a/src/gb/gprs_ns2.c b/src/gb/gprs_ns2.c
index 3eb59e5..09286ec 100644
--- a/src/gb/gprs_ns2.c
+++ b/src/gb/gprs_ns2.c
@@ -236,6 +236,8 @@
 	{ 0, NULL }
 };
 
+static void ns2_free_nse(struct gprs_ns2_nse *nse);
+
 /*! string-format a given NS-VC into a user-supplied buffer.
  *  \param[in] buf user-allocated output buffer
  *  \param[in] buf_len size of user-allocated output buffer in bytes
@@ -631,9 +633,9 @@
  *  \param[in] nsvc NS-VC to destroy */
 void gprs_ns2_free_nsvc(struct gprs_ns2_vc *nsvc)
 {
-	if (!nsvc)
+	if (!nsvc || nsvc->freed)
 		return;
-
+	nsvc->freed = true;
 	ns2_prim_status_ind(nsvc->nse, nsvc, 0, GPRS_NS2_AFF_CAUSE_VC_FAILURE);
 
 	llist_del(&nsvc->list);
@@ -661,14 +663,18 @@
  */
 void gprs_ns2_free_nsvcs(struct gprs_ns2_nse *nse)
 {
-	struct gprs_ns2_vc *nsvc, *tmp;
+	struct gprs_ns2_vc *nsvc;
 
-	if (!nse)
+	if (!nse || nse->freed)
 		return;
 
-	llist_for_each_entry_safe(nsvc, tmp, &nse->nsvc, list) {
+	ns2_get(nse, NSE_USE_LIST);
+loop:
+	llist_for_each_entry(nsvc, &nse->nsvc, list) {
 		gprs_ns2_free_nsvc(nsvc);
+		goto loop;
 	}
+	ns2_put(nse, NSE_USE_LIST);
 }
 
 /*! Allocate a message buffer for use with the NS2 stack. */
@@ -778,6 +784,30 @@
 	return NULL;
 }
 
+static int nse_use_cb(struct osmo_use_count_entry *e, int32_t old_use_count, const char *file, int line)
+{
+	struct gprs_ns2_nse *nse = e->use_count->talloc_object;
+	struct osmo_use_count_entry *e2;
+
+	int32_t other_count = 0;
+	int32_t core_count = 0;
+	if (!e->use)
+		return -EINVAL;
+	if (e->count < 0)
+		return -ERANGE;
+
+	llist_for_each_entry(e2, &nse->use_count.use_counts, entry) {
+		if (strcmp(e2->use, NSE_USE_CORE) == 0)
+			core_count += e2->count;
+		else
+			other_count += e2->count;
+	}
+
+	if (other_count <= 0 && core_count <= 0)
+		ns2_free_nse(nse);
+	return 0;
+}
+
 /*! Create a NS Entity within given NS instance.
  *  \param[in] nsi NS instance in which to create NS Entity
  *  \param[in] nsei NS Entity Identifier of to-be-created NSE
@@ -813,6 +843,11 @@
 	nse->mtu = 0;
 	llist_add_tail(&nse->list, &nsi->nse);
 	INIT_LLIST_HEAD(&nse->nsvc);
+	nse->use_count = (struct osmo_use_count){
+		.talloc_object = nse,
+		.use_cb = nse_use_cb,
+	};
+	ns2_get(nse, NSE_USE_CORE);
 
 	return nse;
 }
@@ -871,26 +906,35 @@
 	return nse->nsei;
 }
 
-/*! Destroy given NS Entity.
- *  \param[in] nse NS Entity to destroy */
-void gprs_ns2_free_nse(struct gprs_ns2_nse *nse)
+static void ns2_free_nse(struct gprs_ns2_nse *nse)
 {
-	if (!nse)
+	struct gprs_ns2_vc *nsvc, *nsvc2;
+	if (!nse || nse->freed)
 		return;
 
+	nse->freed = true;
 	nse->alive = false;
 	if (nse->bss_sns_fi) {
 		osmo_fsm_inst_term(nse->bss_sns_fi, OSMO_FSM_TERM_REQUEST, NULL);
 		nse->bss_sns_fi = NULL;
 	}
 
-	gprs_ns2_free_nsvcs(nse);
-	ns2_prim_status_ind(nse, NULL, 0, GPRS_NS2_AFF_CAUSE_FAILURE);
+	llist_for_each_entry_safe(nsvc, nsvc2, &nse->nsvc, list) {
+		gprs_ns2_free_nsvc(nsvc);
+	}
 
+	ns2_prim_status_ind(nse, NULL, 0, GPRS_NS2_AFF_CAUSE_FAILURE);
 	llist_del(&nse->list);
 	talloc_free(nse);
 }
 
+/*! Destroy given NS Entity.
+ *  \param[in] nse NS Entity to destroy */
+void gprs_ns2_free_nse(struct gprs_ns2_nse *nse)
+{
+	ns2_put(nse, NSE_USE_CORE);
+}
+
 void gprs_ns2_free_nses(struct gprs_ns2_inst *nsi)
 {
 	struct gprs_ns2_nse *nse, *ntmp;
@@ -1240,11 +1284,14 @@
 {
 	struct gprs_ns2_vc *nsvc, *tmp;
 	int rc = 0;
+
+	ns2_get(nse, NSE_USE_LIST);
 	llist_for_each_entry_safe(nsvc, tmp, &nse->nsvc, list) {
 		rc = cb(nsvc, cb_data);
 		if (rc < 0)
 			return rc;
 	}
+	ns2_put(nse, NSE_USE_LIST);
 
 	return 0;
 }
@@ -1450,20 +1497,26 @@
 void gprs_ns2_free_bind(struct gprs_ns2_vc_bind *bind)
 {
 	struct gprs_ns2_vc *nsvc, *tmp;
-	struct gprs_ns2_nse *nse;
-	if (!bind)
+	struct gprs_ns2_nse *nse, *nse2;
+	if (!bind || bind->freed)
 		return;
 
-	llist_for_each_entry_safe(nsvc, tmp, &bind->nsvc, blist) {
-		gprs_ns2_free_nsvc(nsvc);
-	}
+	bind->freed = true;
 
 	if (gprs_ns2_is_ip_bind(bind)) {
-		llist_for_each_entry(nse, &bind->nsi->nse, list) {
+		llist_for_each_entry_safe(nse, nse2, &bind->nsi->nse, list) {
 			gprs_ns2_sns_del_bind(nse, bind);
 		}
 	}
 
+
+	/* because freeing a single nsvc might free all other nsvcs */
+loop:
+	llist_for_each_entry(nsvc, &bind->nsvc, blist) {
+		gprs_ns2_free_nsvc(nsvc);
+		goto loop;
+	}
+
 	if (bind->driver->free_bind)
 		bind->driver->free_bind(bind);
 
diff --git a/src/gb/gprs_ns2_internal.h b/src/gb/gprs_ns2_internal.h
index 70e212a..65cfda6 100644
--- a/src/gb/gprs_ns2_internal.h
+++ b/src/gb/gprs_ns2_internal.h
@@ -6,6 +6,7 @@
 #include <stdint.h>
 
 #include <osmocom/core/logging.h>
+#include <osmocom/core/use_count.h>
 #include <osmocom/gprs/protocol/gsm_08_16.h>
 #include <osmocom/gprs/gprs_ns2.h>
 
@@ -198,6 +199,11 @@
 
 	/*! are we implementing the SGSN role? */
 	bool ip_sns_role_sgsn;
+
+	/*! recursive anchor */
+	bool freed;
+
+	struct osmo_use_count use_count;
 };
 
 /*! Structure representing a single NS-VC */
@@ -242,6 +248,9 @@
 	enum gprs_ns2_vc_mode mode;
 
 	struct osmo_fsm_inst *fi;
+
+	/*! recursive anchor */
+	bool freed;
 };
 
 /*! Structure repesenting a bind instance. E.g. IPv4 listen port. */
@@ -286,6 +295,9 @@
 	uint8_t sns_data_weight;
 
 	struct osmo_stat_item_group *statg;
+
+	/*! recursive anchor */
+	bool freed;
 };
 
 struct gprs_ns2_vc_driver {
@@ -294,6 +306,15 @@
 	void (*free_bind)(struct gprs_ns2_vc_bind *driver);
 };
 
+#define ns2_get(ns2_obj, use) \
+	OSMO_ASSERT(osmo_use_count_get_put(&(ns2_obj)->use_count, use, 1) == 0)
+#define ns2_put(ns2_obj, use) \
+	OSMO_ASSERT(osmo_use_count_get_put(&(ns2_obj)->use_count, use, -1) == 0)
+#define NSE_USE_CORE "ns2core"
+#define NSE_USE_LIST "ns2list"
+#define NSE_USE_SNS "ns2sns"
+
+
 enum ns2_cs ns2_create_vc(struct gprs_ns2_vc_bind *bind,
 			       struct msgb *msg,
 			       const struct osmo_sockaddr *remote,
diff --git a/src/gb/gprs_ns2_sns.c b/src/gb/gprs_ns2_sns.c
index 642b47c..b4a8d7f 100644
--- a/src/gb/gprs_ns2_sns.c
+++ b/src/gb/gprs_ns2_sns.c
@@ -658,6 +658,7 @@
 		OSMO_ASSERT(false);
 	}
 
+	ns2_get(nse, NSE_USE_LIST);
 	llist_for_each_entry_safe(nsvc, tmp, &nse->nsvc, list) {
 		remote = gprs_ns2_ip_vc_remote(nsvc);
 		/* all nsvc in NSE should be IP/UDP nsvc */
@@ -668,6 +669,7 @@
 		LOGPFSML(fi, LOGL_INFO, "DELETE NS-VC %s\n", gprs_ns2_ll_str(nsvc));
 		gprs_ns2_free_nsvc(nsvc);
 	}
+	ns2_put(nse, NSE_USE_LIST);
 
 	return 0;
 }
@@ -1482,6 +1484,7 @@
 	struct ns2_sns_bind *sbind;
 	struct gprs_ns2_vc *nsvc, *nsvc2;
 
+	ns2_get(nse, NSE_USE_SNS);
 	switch (event) {
 	case GPRS_SNS_EV_REQ_ADD_BIND:
 		sbind = data;
@@ -1528,6 +1531,7 @@
 		talloc_free(sbind);
 		break;
 	}
+	ns2_put(nse, NSE_USE_SNS);
 }
 
 /* validate the bss configuration (sns endpoint and binds)
diff --git a/tests/gb/gprs_ns2_test.c b/tests/gb/gprs_ns2_test.c
index 515d908..2322735 100644
--- a/tests/gb/gprs_ns2_test.c
+++ b/tests/gb/gprs_ns2_test.c
@@ -642,6 +642,43 @@
 	printf("--- Finish force unconfigured test\n");
 }
 
+void test_use_count(void *ctx)
+{
+	struct gprs_ns2_inst *nsi;
+	struct gprs_ns2_vc_bind *bind[2];
+	struct gprs_ns2_vc_bind *loopbind;
+	struct gprs_ns2_nse *nse;
+	struct gprs_ns2_vc *nsvc[2];
+	struct gprs_ns2_vc *loop[2];
+
+	struct msgb *msg, *other;
+	char idbuf[32];
+	int i;
+
+	printf("--- Testing nse use count\n");
+	osmo_wqueue_clear(unitdata);
+	printf("---- Create Binds\n");
+	nsi = gprs_ns2_instantiate(ctx, ns_prim_cb, NULL);
+	bind[0] = dummy_bind(nsi, "bblock1");
+	bind[1] = dummy_bind(nsi, "bblock2");
+	loopbind = loopback_bind(nsi, "loopback");
+
+	// free SNS NSE with active NSVCs
+	// free SNS NSE with active NSVCs on a SNS/NSE event
+	// free a dynamic SNS NSE
+
+	nse = gprs_ns2_create_nse(nsi, 1004, GPRS_NS2_LL_UDP, GPRS_NS2_DIALECT_STATIC_RESETBLOCK);
+	OSMO_ASSERT(nse);
+	for (i=0; i<2; i++) {
+		printf("---- Create NSVC[%d]\n", i);
+		snprintf(idbuf, sizeof(idbuf), "NSE%05u-dummy-%i", nse->nsei, i);
+		nsvc[i] = ns2_vc_alloc(bind[i], nse, false, GPRS_NS2_VC_MODE_BLOCKRESET, idbuf);
+		OSMO_ASSERT(nsvc[i]);
+		ns2_vc_fsm_start(nsvc[i]);
+	}
+	gprs_ns2_free_nse(nse);
+}
+
 int main(int argc, char **argv)
 {
 	void *ctx = talloc_named_const(NULL, 0, "gprs_ns2_test");
@@ -663,6 +700,7 @@
 	test_unitdata_weights(ctx);
 	test_unconfigured(ctx);
 	test_mtu(ctx);
+	test_use_count(ctx);
 	printf("===== NS2 protocol test END\n\n");
 
 	talloc_free(ctx);
diff --git a/tests/gb/gprs_ns2_test.ok b/tests/gb/gprs_ns2_test.ok
index 8bae5b9..f4af60c 100644
--- a/tests/gb/gprs_ns2_test.ok
+++ b/tests/gb/gprs_ns2_test.ok
@@ -47,5 +47,9 @@
 ---- Send a small UNITDATA to NSVC[0]
 ---- Check if got mtu reported
 --- Finish unitdata test
+--- Testing nse use count
+---- Create Binds
+---- Create NSVC[0]
+---- Create NSVC[1]
 ===== NS2 protocol test END
 

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/24923
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I03cb2419926c639d4bd357a33ce8008c50cd3bee
Gerrit-Change-Number: 24923
Gerrit-PatchSet: 1
Gerrit-Owner: lynxis lazus <lynxis at fe80.eu>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210712/9248253b/attachment.htm>


More information about the gerrit-log mailing list