From 986a6617cc9425dfa4f9fcc879a4d98a3ab00b7c Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 7 Aug 2019 13:47:34 -0400 Subject: [PATCH] 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 --- lib/nexthop.c | 13 ++++++ lib/nexthop.h | 2 +- zebra/zebra_rib.c | 112 ++++++++++++++-------------------------------- 3 files changed, 47 insertions(+), 80 deletions(-) diff --git a/lib/nexthop.c b/lib/nexthop.c index da7c934efa..3ab7284492 100644 --- a/lib/nexthop.c +++ b/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; diff --git a/lib/nexthop.h b/lib/nexthop.h index 5558e857f6..dfb30a1bce 100644 --- a/lib/nexthop.h +++ b/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, diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 52cc019d7c..d4abb61f22 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1421,77 +1421,21 @@ static bool rib_update_re_from_ctx(struct route_entry *re, * status. */ - /* - * First check the fib nexthop-group, if it's present. The comparison - * here is quite strict: we require that the fib sets match exactly. + /* Check both fib group and notif group for equivalence. + * + * Let's assume the nexthops are ordered here to save time. */ - matched = false; - do { - if (re->fib_ng.nexthop == NULL) - break; + if (nexthop_group_equal(&re->fib_ng, dplane_ctx_get_ng(ctx)) == false) { + if (IS_ZEBRA_DEBUG_RIB_DETAILED) { + zlog_debug( + "%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; - for (ALL_NEXTHOPS_PTR(dplane_ctx_get_ng(ctx), ctx_nexthop)) { - if (nexthop_same(ctx_nexthop, nexthop)) - break; - } - - /* If the FIB doesn't know about the nexthop, - * it's not installed - */ - if (ctx_nexthop == NULL) { + if (nexthop_same(ctx_nexthop, nexthop) == false) { + /* If the FIB doesn't know about the nexthop, + * it's not installed + */ 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", - nh_str); + zlog_debug( + "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 */