Merge pull request #5452 from mjstapp/fix_notify_nhg

zebra: align dplane notify processing with nhg work

4 files changed, 124 insertions, 34 deletions

diff --git a/lib/nexthop_group.c b/lib/nexthop_group.c
index 991843a04..6d64c1ca1 100644
--- a/lib/nexthop_group.c
+++ b/lib/nexthop_group.c
@@ -244,25 +244,16 @@ void _nexthop_add(struct nexthop **target, struct nexthop *nexthop)
 	nexthop->prev = last;
 }
 
-void nexthop_group_add_sorted(struct nexthop_group *nhg,
-			      struct nexthop *nexthop)
+/* Add nexthop to sorted list of nexthops */
+static void _nexthop_add_sorted(struct nexthop **head,
+				struct nexthop *nexthop)
 {
-	struct nexthop *position, *prev, *tail;
-
-	/* Try to just append to the end first
-	 * This trust it is already sorted
-	 */
-
-	tail = nexthop_group_tail(nhg);
+	struct nexthop *position, *prev;
 
-	if (tail && (nexthop_cmp(tail, nexthop) < 0)) {
-		tail->next = nexthop;
-		nexthop->prev = tail;
-
-		return;
-	}
+	/* Ensure this gets set */
+	nexthop->next = NULL;
 
-	for (position = nhg->nexthop, prev = NULL; position;
+	for (position = *head, prev = NULL; position;
 	     prev = position, position = position->next) {
 		if (nexthop_cmp(position, nexthop) > 0) {
 			nexthop->next = position;
@@ -271,7 +262,7 @@ void nexthop_group_add_sorted(struct nexthop_group *nhg,
 			if (nexthop->prev)
 				nexthop->prev->next = nexthop;
 			else
-				nhg->nexthop = nexthop;
+				*head = nexthop;
 
 			position->prev = nexthop;
 			return;
@@ -282,7 +273,27 @@ void nexthop_group_add_sorted(struct nexthop_group *nhg,
 	if (prev)
 		prev->next = nexthop;
 	else
-		nhg->nexthop = nexthop;
+		*head = nexthop;
+}
+
+void nexthop_group_add_sorted(struct nexthop_group *nhg,
+			      struct nexthop *nexthop)
+{
+	struct nexthop *tail;
+
+	/* Try to just append to the end first;
+	 * trust the list is already sorted
+	 */
+	tail = nexthop_group_tail(nhg);
+
+	if (tail && (nexthop_cmp(tail, nexthop) < 0)) {
+		tail->next = nexthop;
+		nexthop->prev = tail;
+
+		return;
+	}
+
+	_nexthop_add_sorted(&nhg->nexthop, nexthop);
 }
 
 /* Delete nexthop from a nexthop list.  */
@@ -309,6 +320,40 @@ void _nexthop_del(struct nexthop_group *nhg, struct nexthop *nh)
 	nh->next = NULL;
 }
 
+/*
+ * Copy a list of nexthops in 'nh' to an nhg, enforcing canonical sort order
+ */
+void nexthop_group_copy_nh_sorted(struct nexthop_group *nhg,
+				  const struct nexthop *nh)
+{
+	struct nexthop *nexthop, *tail;
+	const struct nexthop *nh1;
+
+	/* We'll try to append to the end of the new list;
+	 * if the original list in nh is already sorted, this eliminates
+	 * lots of comparison operations.
+	 */
+	tail = nexthop_group_tail(nhg);
+
+	for (nh1 = nh; nh1; nh1 = nh1->next) {
+		nexthop = nexthop_dup(nh1, NULL);
+
+		if (tail && (nexthop_cmp(tail, nexthop) < 0)) {
+			tail->next = nexthop;
+			nexthop->prev = tail;
+
+			tail = nexthop;
+			continue;
+		}
+
+		_nexthop_add_sorted(&nhg->nexthop, nexthop);
+
+		if (tail == NULL)
+			tail = nexthop;
+	}
+}
+
+/* Copy a list of nexthops, no effort made to sort or order them. */
 void copy_nexthops(struct nexthop **tnh, const struct nexthop *nh,
 		   struct nexthop *rparent)
 {
diff --git a/lib/nexthop_group.h b/lib/nexthop_group.h
index c90b21737..73b020283 100644
--- a/lib/nexthop_group.h
+++ b/lib/nexthop_group.h
@@ -44,6 +44,13 @@ void nexthop_group_delete(struct nexthop_group **nhg);
 
 void nexthop_group_copy(struct nexthop_group *to,
 			struct nexthop_group *from);
+
+/*
+ * Copy a list of nexthops in 'nh' to an nhg, enforcing canonical sort order
+ */
+void nexthop_group_copy_nh_sorted(struct nexthop_group *nhg,
+				  const struct nexthop *nh);
+
 void copy_nexthops(struct nexthop **tnh, const struct nexthop *nh,
 		   struct nexthop *rparent);
 
diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c
index 417c20646..bf1ba522a 100644
--- a/zebra/zebra_dplane.c
+++ b/zebra/zebra_dplane.c
@@ -1023,6 +1023,11 @@ uint8_t dplane_ctx_get_old_distance(const struct zebra_dplane_ctx *ctx)
 	return ctx->u.rinfo.zd_old_distance;
 }
 
+/*
+ * Set the nexthops associated with a context: note that processing code
+ * may well expect that nexthops are in canonical (sorted) order, so we
+ * will enforce that here.
+ */
 void dplane_ctx_set_nexthops(struct zebra_dplane_ctx *ctx, struct nexthop *nh)
 {
 	DPLANE_CTX_VALID(ctx);
@@ -1031,7 +1036,7 @@ void dplane_ctx_set_nexthops(struct zebra_dplane_ctx *ctx, struct nexthop *nh)
 		nexthops_free(ctx->u.rinfo.zd_ng.nexthop);
 		ctx->u.rinfo.zd_ng.nexthop = NULL;
 	}
-	copy_nexthops(&(ctx->u.rinfo.zd_ng.nexthop), nh, NULL);
+	nexthop_group_copy_nh_sorted(&(ctx->u.rinfo.zd_ng), nh);
 }
 
 const struct nexthop_group *dplane_ctx_get_ng(
diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index d525da26e..f375036db 100644
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -1369,6 +1369,14 @@ static bool rib_update_re_from_ctx(struct route_entry *re,
 
 	ctx_nexthop = dplane_ctx_get_ng(ctx)->nexthop;
 
+	/* Nothing installed - we can skip some of the checking/comparison
+	 * of nexthops.
+	 */
+	if (ctx_nexthop == NULL) {
+		changed_p = true;
+		goto no_nexthops;
+	}
+
 	/* 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->nhe->nhg?
@@ -1431,6 +1439,8 @@ static bool rib_update_re_from_ctx(struct route_entry *re,
 		goto done;
 	}
 
+no_nexthops:
+
 	/* FIB nexthop set differs from the RIB set:
 	 * create a fib-specific nexthop-group
 	 */
@@ -1788,18 +1798,40 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx)
 	/* Ensure we clear the QUEUED flag */
 	UNSET_FLAG(re->status, ROUTE_ENTRY_QUEUED);
 
-	/* Is this a notification that ... matters? We only really care about
-	 * the route that is currently selected for installation.
+	/* Is this a notification that ... matters? We mostly care about
+	 * the route that is currently selected for installation; we may also
+	 * get an un-install notification, and handle that too.
 	 */
 	if (re != dest->selected_fib) {
-		/* TODO -- don't skip processing entirely? We might like to
-		 * at least report on the event.
+		/*
+		 * If we need to, clean up after a delete that was part of
+		 * an update operation.
 		 */
-		if (debug_p)
-			zlog_debug("%u:%s dplane notif, but type %s not selected_fib",
-				   dplane_ctx_get_vrf(ctx), dest_str,
-				   zebra_route_string(
-					   dplane_ctx_get_type(ctx)));
+		end_count = 0;
+		for (ALL_NEXTHOPS_PTR(dplane_ctx_get_ng(ctx), nexthop)) {
+			if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_FIB))
+				end_count++;
+		}
+
+		/* If no nexthops or none installed, ensure that this re
+		 * gets its 'installed' flag cleared.
+		 */
+		if (end_count == 0) {
+			if (CHECK_FLAG(re->status, ROUTE_ENTRY_INSTALLED))
+				UNSET_FLAG(re->status, ROUTE_ENTRY_INSTALLED);
+			if (debug_p)
+				zlog_debug("%u:%s dplane notif, uninstalled type %s route",
+					   dplane_ctx_get_vrf(ctx), dest_str,
+					   zebra_route_string(
+						   dplane_ctx_get_type(ctx)));
+		} else {
+			/* At least report on the event. */
+			if (debug_p)
+				zlog_debug("%u:%s dplane notif, but type %s not selected_fib",
+					   dplane_ctx_get_vrf(ctx), dest_str,
+					   zebra_route_string(
+						   dplane_ctx_get_type(ctx)));
+		}
 		goto done;
 	}
 
@@ -1808,9 +1840,12 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx)
 	 * and then again if there's been a change.
 	 */
 	start_count = 0;
-	for (ALL_NEXTHOPS_PTR(rib_active_nhg(re), nexthop)) {
-		if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_FIB))
-			start_count++;
+
+	if (CHECK_FLAG(re->status, ROUTE_ENTRY_INSTALLED)) {
+		for (ALL_NEXTHOPS_PTR(rib_active_nhg(re), nexthop)) {
+			if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_FIB))
+				start_count++;
+		}
 	}
 
 	/* Update zebra's nexthop FIB flags based on the context struct's
@@ -1820,10 +1855,8 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx)
 
 	if (!fib_changed) {
 		if (debug_p)
-			zlog_debug("%u:%s No change from dplane notification",
+			zlog_debug("%u:%s dplane notification: rib_update returns FALSE",
 				   dplane_ctx_get_vrf(ctx), dest_str);
-
-		goto done;
 	}
 
 	/*