zebra: Optimize the fib/notified nexthop matching

Optimize the fib and notified nexthop group comparison algorithm
to assume ordering. There were some pretty serious performance hits with
this on high ecmp routes.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>

lib/nexthop.c

@@ -364,6 +364,19 @@ struct nexthop *nexthop_next(struct nexthop *nexthop)
 	return NULL;
 }
 
+/* Return the next nexthop in the tree that is resolved and active */
+struct nexthop *nexthop_next_active_resolved(struct nexthop *nexthop)
+{
+	struct nexthop *next = nexthop_next(nexthop);
+
+	while (next
+	       && (CHECK_FLAG(next->flags, NEXTHOP_FLAG_RECURSIVE)
+		   || !CHECK_FLAG(next->flags, NEXTHOP_FLAG_ACTIVE)))
+		next = nexthop_next(next);
+
+	return next;
+}
+
 unsigned int nexthop_level(struct nexthop *nexthop)
 {
 	unsigned int rv = 0;

lib/nexthop.h

@@ -154,7 +154,7 @@ extern int nexthop_same_firsthop(struct nexthop *next1, struct nexthop *next2);
 extern const char *nexthop2str(const struct nexthop *nexthop,
 			       char *str, int size);
 extern struct nexthop *nexthop_next(struct nexthop *nexthop);
-extern struct nexthop *nexthop_recursive_next(struct nexthop *nexthop);
+extern struct nexthop *nexthop_next_active_resolved(struct nexthop *nexthop);
 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,

zebra/zebra_rib.c

@@ -1421,77 +1421,21 @@ static bool rib_update_re_from_ctx(struct route_entry *re,
 	 * status.
 	 */
 
-	/*
+	/* Check both fib group and notif group for equivalence.
-	 * First check the fib nexthop-group, if it's present. The comparison
+	 *
-	 * here is quite strict: we require that the fib sets match exactly.
+	 * Let's assume the nexthops are ordered here to save time.
 	 */
-	matched = false;
+	if (nexthop_group_equal(&re->fib_ng, dplane_ctx_get_ng(ctx)) == false) {
-	do {
+		if (IS_ZEBRA_DEBUG_RIB_DETAILED) {
-		if (re->fib_ng.nexthop == NULL)
+			zlog_debug(
-			break;
+				"%u:%s update_from_ctx: notif nh and fib nh mismatch",
+				re->vrf_id, dest_str);
+		}
 
+		matched = false;
+	} else
 		matched = true;
 
-		/* First check the route's fib nexthops */
-		for (ALL_NEXTHOPS(re->fib_ng, nexthop)) {
-
-			if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_RECURSIVE))
-				continue;
-
-			ctx_nexthop = NULL;
-			for (ALL_NEXTHOPS_PTR(dplane_ctx_get_ng(ctx),
-					      ctx_nexthop)) {
-				if (nexthop_same(ctx_nexthop, nexthop))
-					break;
-			}
-
-			if (ctx_nexthop == NULL) {
-				/* Nexthop not in the new installed set */
-				if (IS_ZEBRA_DEBUG_RIB_DETAILED) {
-					nexthop2str(nexthop, nh_str,
-						    sizeof(nh_str));
-					zlog_debug("update_from_ctx: no match for fib nh %s",
-						   nh_str);
-				}
-
-				matched = false;
-				break;
-			}
-		}
-
-		if (!matched)
-			break;
-
-		/* Check the new installed set */
-		ctx_nexthop = NULL;
-		for (ALL_NEXTHOPS_PTR(dplane_ctx_get_ng(ctx), ctx_nexthop)) {
-
-			if (CHECK_FLAG(ctx_nexthop->flags,
-				       NEXTHOP_FLAG_RECURSIVE))
-				continue;
-
-			/* Compare with the current group's nexthops */
-			nexthop = NULL;
-			for (ALL_NEXTHOPS(re->fib_ng, nexthop)) {
-				if (nexthop_same(nexthop, ctx_nexthop))
-					break;
-			}
-
-			if (nexthop == NULL) {
-				/* Nexthop not in the old installed set */
-				if (IS_ZEBRA_DEBUG_RIB_DETAILED) {
-					nexthop2str(ctx_nexthop, nh_str,
-						    sizeof(nh_str));
-					zlog_debug("update_from_ctx: no fib match for notif nh %s",
-						   nh_str);
-				}
-				matched = false;
-				break;
-			}
-		}
-
-	} while (0);
-
 	/* If the new FIB set matches the existing FIB set, we're done. */
 	if (matched) {
 		if (IS_ZEBRA_DEBUG_RIB)
@@ -1523,8 +1467,21 @@ static bool rib_update_re_from_ctx(struct route_entry *re,
 	 * walk the RIB group, looking for the 'installable' candidate
 	 * nexthops, and then check those against the set
 	 * that is actually installed.
+	 *
+	 * Assume nexthops are ordered here as well.
 	 */
 	matched = true;
+
+	ctx_nexthop = dplane_ctx_get_ng(ctx)->nexthop;
+
+	/* Get the first `installed` one to check against.
+	 * If the dataplane doesn't set these to be what was actually installed,
+	 * it will just be whatever was in re->ng?
+	 */
+	if (CHECK_FLAG(ctx_nexthop->flags, NEXTHOP_FLAG_RECURSIVE)
+	    || !CHECK_FLAG(ctx_nexthop->flags, NEXTHOP_FLAG_ACTIVE))
+		ctx_nexthop = nexthop_next_active_resolved(ctx_nexthop);
+
 	for (ALL_NEXTHOPS_PTR(re->ng, nexthop)) {
 
 		if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_RECURSIVE))
@@ -1534,20 +1491,15 @@ static bool rib_update_re_from_ctx(struct route_entry *re,
 			continue;
 
 		/* Check for a FIB nexthop corresponding to the RIB nexthop */
-		ctx_nexthop = NULL;
+		if (nexthop_same(ctx_nexthop, nexthop) == false) {
-		for (ALL_NEXTHOPS_PTR(dplane_ctx_get_ng(ctx), ctx_nexthop)) {
+			/* If the FIB doesn't know about the nexthop,
-			if (nexthop_same(ctx_nexthop, nexthop))
+			 * it's not installed
-				break;
+			 */
-		}
-
-		/* If the FIB doesn't know about the nexthop,
-		 * it's not installed
-		 */
-		if (ctx_nexthop == NULL) {
 			if (IS_ZEBRA_DEBUG_RIB_DETAILED) {
 				nexthop2str(nexthop, nh_str, sizeof(nh_str));
-				zlog_debug("update_from_ctx: no notif match for rib nh %s",
+				zlog_debug(
-					   nh_str);
+					"update_from_ctx: no notif match for rib nh %s",
+					nh_str);
 			}
 			matched = false;
 
@@ -1571,6 +1523,8 @@ static bool rib_update_re_from_ctx(struct route_entry *re,
 
 			UNSET_FLAG(nexthop->flags, NEXTHOP_FLAG_FIB);
 		}
+
+		ctx_nexthop = nexthop_next_active_resolved(ctx_nexthop);
 	}
 
 	/* If all nexthops were processed, we're done */