Merge pull request #5600 from sworleys/NHG-Depend-Crash

zebra: can't improve efficiency for recursive depends

lib/nexthop.c

@@ -34,6 +34,7 @@
 #include "jhash.h"
 #include "printfrr.h"
 #include "vrf.h"
+#include "nexthop_group.h"
 
 DEFINE_MTYPE_STATIC(LIB, NEXTHOP, "Nexthop")
 DEFINE_MTYPE_STATIC(LIB, NH_LABEL, "Nexthop label")
@@ -565,8 +566,9 @@ uint32_t nexthop_hash(const struct nexthop *nexthop)
 	return key;
 }
 
-void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop,
+void nexthop_copy_no_recurse(struct nexthop *copy,
-		  struct nexthop *rparent)
+			     const struct nexthop *nexthop,
+			     struct nexthop *rparent)
 {
 	copy->vrf_id = nexthop->vrf_id;
 	copy->ifindex = nexthop->ifindex;
@@ -583,6 +585,28 @@ void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop,
 				   &nexthop->nh_label->label[0]);
 }
 
+void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop,
+		  struct nexthop *rparent)
+{
+	nexthop_copy_no_recurse(copy, nexthop, rparent);
+
+	/* Bit of a special case here, we need to handle the case
+	 * of a nexthop resolving to agroup. Hence, we need to
+	 * use a nexthop_group API.
+	 */
+	if (CHECK_FLAG(copy->flags, NEXTHOP_FLAG_RECURSIVE))
+		copy_nexthops(&copy->resolved, nexthop->resolved, copy);
+}
+
+struct nexthop *nexthop_dup_no_recurse(const struct nexthop *nexthop,
+				       struct nexthop *rparent)
+{
+	struct nexthop *new = nexthop_new();
+
+	nexthop_copy_no_recurse(new, nexthop, rparent);
+	return new;
+}
+
 struct nexthop *nexthop_dup(const struct nexthop *nexthop,
 			    struct nexthop *rparent)
 {

lib/nexthop.h

@@ -187,9 +187,16 @@ extern unsigned int nexthop_level(struct nexthop *nexthop);
 /* Copies to an already allocated nexthop struct */
 extern void nexthop_copy(struct nexthop *copy, const struct nexthop *nexthop,
 			 struct nexthop *rparent);
+/* Copies to an already allocated nexthop struct, not including recurse info */
+extern void nexthop_copy_no_recurse(struct nexthop *copy,
+				    const struct nexthop *nexthop,
+				    struct nexthop *rparent);
 /* Duplicates a nexthop and returns the newly allocated nexthop */
 extern struct nexthop *nexthop_dup(const struct nexthop *nexthop,
 				   struct nexthop *rparent);
+/* Duplicates a nexthop and returns the newly allocated nexthop */
+extern struct nexthop *nexthop_dup_no_recurse(const struct nexthop *nexthop,
+					      struct nexthop *rparent);
 
 #ifdef __cplusplus
 }

lib/nexthop_group.c

@@ -364,10 +364,6 @@ void copy_nexthops(struct nexthop **tnh, const struct nexthop *nh,
 	for (nh1 = nh; nh1; nh1 = nh1->next) {
 		nexthop = nexthop_dup(nh1, rparent);
 		_nexthop_add(tnh, nexthop);
-
-		if (CHECK_FLAG(nh1->flags, NEXTHOP_FLAG_RECURSIVE))
-			copy_nexthops(&nexthop->resolved, nh1->resolved,
-				      nexthop);
 	}
 }
 

zebra/zebra_nhg.c

@@ -1048,25 +1048,55 @@ int zebra_nhg_kernel_del(uint32_t id)
 }
 
 /* Some dependency helper functions */
-static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi)
+static struct nhg_hash_entry *depends_find_recursive(const struct nexthop *nh,
+						     afi_t afi)
 {
-	struct nexthop lookup;
+	struct nhg_hash_entry *nhe;
-	struct nhg_hash_entry *nhe = NULL;
+	struct nexthop *lookup = NULL;
 
-	if (!nh)
+	lookup = nexthop_dup(nh, NULL);
-		goto done;
+
+	nhe = zebra_nhg_find_nexthop(0, lookup, afi, 0);
+
+	nexthops_free(lookup);
+
+	return nhe;
+}
+
+static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh,
+						     afi_t afi)
+{
+	struct nhg_hash_entry *nhe;
+	struct nexthop lookup = {};
 
 	/* Capture a snapshot of this single nh; it might be part of a list,
 	 * so we need to make a standalone copy.
 	 */
-	memset(&lookup, 0, sizeof(lookup));
+	nexthop_copy_no_recurse(&lookup, nh, NULL);
-	nexthop_copy(&lookup, nh, NULL);
 
 	nhe = zebra_nhg_find_nexthop(0, &lookup, afi, 0);
 
 	/* The copy may have allocated labels; free them if necessary. */
 	nexthop_del_labels(&lookup);
 
+	return nhe;
+}
+
+static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi)
+{
+	struct nhg_hash_entry *nhe = NULL;
+
+	if (!nh)
+		goto done;
+
+	/* We are separating these functions out to increase handling speed
+	 * in the non-recursive case (by not alloc/freeing)
+	 */
+	if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE))
+		nhe = depends_find_recursive(nh, afi);
+	else
+		nhe = depends_find_singleton(nh, afi);
+
 done:
 	return nhe;
 }