nexthop: generalize reference removal

When a nexthop is destroyed, other nexthops may hold references to it.
Previously, nexthop_destroy() had hardcoded logic to remove the deleted
nexthop from group members. This does not scale as more nexthop types
with cross-references are added.

Introduce a remove_references callback in nexthop_type_ops. When
destroying a nexthop, iterate through all registered types and invoke
their callback if present. Move the L3 hash key removal from free to
remove_references. Implement the callback for group nexthops to remove
deleted members and for DNAT nexthops to clean up SNAT policies.

Signed-off-by: Robin Jarry <rjarry@redhat.com>
Reviewed-by: Christophe Fontaine <cfontain@redhat.com>

Author: rjarry

modules/infra/control/gr_nh_control.h

@@ -141,6 +141,7 @@ struct nexthop_type_ops {
 	int (*reconfig)(const struct gr_nexthop_config *);
 	struct nexthop *(*lookup)(const struct gr_nexthop_base *, const void *info);
 	// Callback that will be invoked the nexthop refcount reaches zero.
+	void (*remove_references)(struct nexthop *);
 	void (*free)(struct nexthop *);
 	bool (*equal)(const struct nexthop *, const struct nexthop *);
 	// Copy public info structure to internal info structure.

modules/infra/control/group_nexthop.c

@@ -22,6 +22,24 @@ static bool group_equal(const struct nexthop *a, const struct nexthop *b) {
 	return true;
 }
 
+static void remove_group_member_cb(struct nexthop *nh, void *deleted) {
+	if (nh->type != GR_NH_T_GROUP)
+		return;
+
+	struct nexthop_info_group *g = nexthop_info_group(nh);
+	for (uint32_t i = 0; i < g->n_members; i++) {
+		if (g->members[i].nh == deleted) {
+			g->members[i].nh = g->members[g->n_members - 1].nh;
+			g->members[i].weight = g->members[g->n_members - 1].weight;
+			g->n_members--;
+		}
+	}
+}
+
+static void group_remove_references(struct nexthop *nh) {
+	nexthop_iter(remove_group_member_cb, nh);
+}
+
 static void group_free(struct nexthop *nh) {
 	struct nexthop_info_group *pvt = nexthop_info_group(nh);
 
@@ -174,6 +192,7 @@ static struct gr_nexthop *group_to_api(const struct nexthop *nh, size_t *len) {
 
 static struct nexthop_type_ops group_nh_ops = {
 	.equal = group_equal,
+	.remove_references = group_remove_references,
 	.free = group_free,
 	.import_info = group_import_info,
 	.to_api = group_to_api,

modules/infra/control/l3_nexthop.c

@@ -130,15 +130,21 @@ void nexthop_routes_cleanup(struct nexthop *nh) {
 	}
 }
 
+static void l3_remove_references(struct nexthop *nh) {
+	if (nh->type == GR_NH_T_L3) {
+		struct nexthop_info_l3 *l3 = nexthop_info_l3(nh);
+
+		if (l3->ipv4 != 0 || !rte_ipv6_addr_is_unspec(&l3->ipv6)) {
+			struct nexthop_key key;
+			set_nexthop_key(&key, l3->af, nh->vrf_id, nh->iface_id, &l3->addr);
+			rte_hash_del_key(l3_hash, &key);
+		}
+	}
+}
+
 static void l3_free(struct nexthop *nh) {
 	struct nexthop_info_l3 *l3 = nexthop_info_l3(nh);
 
-	if (l3->ipv4 != 0 || !rte_ipv6_addr_is_unspec(&l3->ipv6)) {
-		struct nexthop_key key;
-		set_nexthop_key(&key, l3->af, nh->vrf_id, nh->iface_id, &l3->addr);
-		rte_hash_del_key(l3_hash, &key);
-	}
-
 	// Flush all held packets.
 	struct rte_mbuf *m = l3->held_pkts_head;
 	while (m != NULL) {
@@ -258,6 +264,7 @@ static struct gr_nexthop *l3_to_api(const struct nexthop *nh, size_t *len) {
 static struct nexthop_type_ops l3_nh_ops = {
 	.reconfig = l3_reconfig,
 	.lookup = l3_lookup,
+	.remove_references = l3_remove_references,
 	.free = l3_free,
 	.equal = l3_equal,
 	.import_info = l3_import_info,

modules/infra/control/nexthop.c

@@ -438,28 +438,6 @@ struct nexthop *nexthop_lookup_id(uint32_t nh_id) {
 	return data;
 }
 
-static void nh_groups_remove_member(const struct nexthop *nh) {
-	struct nexthop_info_group *info;
-	struct nexthop *group;
-	uint32_t next = 0;
-	const void *key;
-	void *data;
-
-	while (rte_hash_iterate(hash_by_id, &key, &data, &next) >= 0) {
-		group = data;
-		if (group->type != GR_NH_T_GROUP)
-			continue;
-		info = nexthop_info_group(group);
-		for (uint32_t i = 0; i < info->n_members; i++) {
-			if (info->members[i].nh == nh) {
-				info->members[i].nh = info->members[info->n_members - 1].nh;
-				info->members[i].weight = info->members[info->n_members - 1].weight;
-				info->n_members--;
-			}
-		}
-	}
-}
-
 static void nh_cleanup_interface_cb(struct nexthop *nh, void *priv) {
 	if (nh->iface_id == (uintptr_t)priv) {
 		nexthop_routes_cleanup(nh);
@@ -480,10 +458,17 @@ static struct gr_event_subscription iface_subscription = {
 };
 
 void nexthop_destroy(struct nexthop *nh) {
+	const struct nexthop_type_ops *ops;
+
 	assert(nh->ref_count == 0);
 
-	nh_groups_remove_member(nh);
+	for (gr_nh_type_t t = 0; nexthop_type_valid(t); t++) {
+		ops = type_ops[t];
+		if (ops != NULL && ops->remove_references != NULL)
+			ops->remove_references(nh);
+	}
 	nexthop_id_put(nh);
+
 	rte_rcu_qsbr_synchronize(gr_datapath_rcu(), RTE_QSBR_THRID_INVALID);
 
 	// Push NEXTHOP_DELETE event after RCU sync to ensure all datapath
@@ -494,7 +479,7 @@ void nexthop_destroy(struct nexthop *nh) {
 	if (nh->origin != GR_NH_ORIGIN_INTERNAL)
 		gr_event_push(GR_EVENT_NEXTHOP_DELETE, nh);
 
-	const struct nexthop_type_ops *ops = type_ops[nh->type];
+	ops = type_ops[nh->type];
 	if (ops != NULL && ops->free != NULL)
 		ops->free(nh);
 	rte_mempool_put(pool, nh);

modules/policy/api/dnat44.c

@@ -31,17 +31,21 @@ static bool dnat44_nh_equal(const struct nexthop *a, const struct nexthop *b) {
 	return ad->match == bd->match && ad->replace == bd->replace;
 }
 
+static void dnat44_nh_remove_references(struct nexthop *nh) {
+	if (nh->type == GR_NH_T_DNAT) {
+		struct nexthop_info_dnat *dnat = nexthop_info_dnat(nh);
+		struct iface *iface = iface_from_id(nh->iface_id);
+		if (iface == NULL)
+			return;
+		snat44_static_policy_del(iface, dnat->replace);
+	}
+}
+
 static void dnat44_nh_free(struct nexthop *nh) {
 	struct nexthop_info_dnat *dnat = nexthop_info_dnat(nh);
-	struct iface *iface;
-
-	nexthop_decref(dnat->arp);
-
-	iface = iface_from_id(nh->iface_id);
-	if (iface == NULL)
-		return;
-
-	snat44_static_policy_del(iface, dnat->replace);
+	if (dnat->arp)
+		nexthop_decref(dnat->arp);
+	dnat->arp = NULL;
 }
 
 static int dnat44_nh_import_info(struct nexthop *nh, const void *info) {
@@ -240,6 +244,7 @@ static struct gr_api_handler list_handler = {
 static struct nexthop_type_ops nh_ops = {
 	.lookup = dnat44_nh_lookup,
 	.equal = dnat44_nh_equal,
+	.remove_references = dnat44_nh_remove_references,
 	.free = dnat44_nh_free,
 	.import_info = dnat44_nh_import_info,
 	.to_api = dnat44_nh_to_api,