From b8e24a0ad07b9190cd6a6d1f9f36bb8805193b48 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 20 Aug 2024 08:28:17 -0400 Subject: [PATCH 1/2] lib, zebra: Modify nexthop_cmp to allow you to use weight or not Currently nexthop weight is a discriminator on whether or not a nexthop matches. There is a need to no use the weight as part of this comparison function so let's add a boolean to allow us to say use this or not. Signed-off-by: Donald Sharp --- lib/nexthop.c | 30 ++++++++++++++++++++++-------- lib/nexthop.h | 2 ++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/nexthop.c b/lib/nexthop.c index 65c12c1e6931..ac22e7ec8451 100644 --- a/lib/nexthop.c +++ b/lib/nexthop.c @@ -139,7 +139,7 @@ static int _nexthop_source_cmp(const struct nexthop *nh1, } static int _nexthop_cmp_no_labels(const struct nexthop *next1, - const struct nexthop *next2) + const struct nexthop *next2, bool use_weight) { int ret = 0; @@ -155,11 +155,13 @@ static int _nexthop_cmp_no_labels(const struct nexthop *next1, if (next1->type > next2->type) return 1; - if (next1->weight < next2->weight) - return -1; + if (use_weight) { + if (next1->weight < next2->weight) + return -1; - if (next1->weight > next2->weight) - return 1; + if (next1->weight > next2->weight) + return 1; + } switch (next1->type) { case NEXTHOP_TYPE_IPV4: @@ -227,11 +229,12 @@ static int _nexthop_cmp_no_labels(const struct nexthop *next1, return ret; } -int nexthop_cmp(const struct nexthop *next1, const struct nexthop *next2) +static int nexthop_cmp_internal(const struct nexthop *next1, + const struct nexthop *next2, bool use_weight) { int ret = 0; - ret = _nexthop_cmp_no_labels(next1, next2); + ret = _nexthop_cmp_no_labels(next1, next2, use_weight); if (ret != 0) return ret; @@ -244,6 +247,17 @@ int nexthop_cmp(const struct nexthop *next1, const struct nexthop *next2) return ret; } +int nexthop_cmp(const struct nexthop *next1, const struct nexthop *next2) +{ + return nexthop_cmp_internal(next1, next2, true); +} + +int nexthop_cmp_no_weight(const struct nexthop *next1, + const struct nexthop *next2) +{ + return nexthop_cmp_internal(next1, next2, false); +} + /* * More-limited comparison function used to detect duplicate * nexthops. This is used in places where we don't need the full @@ -441,7 +455,7 @@ bool nexthop_same_no_labels(const struct nexthop *nh1, if (nh1 == nh2) return true; - if (_nexthop_cmp_no_labels(nh1, nh2) != 0) + if (_nexthop_cmp_no_labels(nh1, nh2, true) != 0) return false; return true; diff --git a/lib/nexthop.h b/lib/nexthop.h index 27073b948dea..15cfb26d8206 100644 --- a/lib/nexthop.h +++ b/lib/nexthop.h @@ -207,6 +207,8 @@ extern bool nexthop_same(const struct nexthop *nh1, const struct nexthop *nh2); extern bool nexthop_same_no_labels(const struct nexthop *nh1, const struct nexthop *nh2); extern int nexthop_cmp(const struct nexthop *nh1, const struct nexthop *nh2); +extern int nexthop_cmp_no_weight(const struct nexthop *nh1, + const struct nexthop *nh2); extern int nexthop_g_addr_cmp(enum nexthop_types_t type, const union g_addr *addr1, const union g_addr *addr2); From c20fa972d68df9c25f69726ce808139b2aadbe63 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 20 Aug 2024 10:53:34 -0400 Subject: [PATCH 2/2] zebra: Create Singleton nhg's without weights Currently FRR when it has two nexthop groups: A nexthop 1 weight 5 nexthop 2 weight 6 nexthop 3 weight 7 B nexthop 1 weight 3 nexthop 2 weight 4 nexthop 3 weight 5 We end up with 5 singleton nexthops and two groups: ID: 181818168 (sharp) RefCnt: 1 Uptime: 00:04:52 VRF: default Valid, Installed Depends: (69) (70) (71) via 192.168.119.1, enp13s0 (vrf default), weight 182 via 192.168.119.2, enp13s0 (vrf default), weight 218 via 192.168.119.3, enp13s0 (vrf default), weight 255 ID: 181818169 (sharp) RefCnt: 1 Uptime: 00:02:08 VRF: default Valid, Installed Depends: (71) (127) (128) via 192.168.119.1, enp13s0 (vrf default), weight 127 via 192.168.119.2, enp13s0 (vrf default), weight 170 via 192.168.119.3, enp13s0 (vrf default), weight 255 id 69 via 192.168.119.1 dev enp13s0 scope link proto 194 id 70 via 192.168.119.2 dev enp13s0 scope link proto 194 id 71 via 192.168.119.3 dev enp13s0 scope link proto 194 id 127 via 192.168.119.1 dev enp13s0 scope link proto 194 id 128 via 192.168.119.2 dev enp13s0 scope link proto 194 id 181818168 group 69,182/70,218/71,255 proto 194 id 181818169 group 71,255/127,127/128,170 proto 194 This is not a desirable state to be in. If you have a link flapping in the network and weights are changing rapidly you end up with a large number of singleton nexthops that are being used by the nexthop groups. This fills up asic space and clutters the table. Additionally singleton nexthops cannot have any weight and the fact that you attempt to create a singleton nexthop with different weights means nothing to the linux kernel( or any asic dplane ). Let's modify the code to always create the singleton nexthops without a weight and then just creating the NHG's that use the singletons with the appropriate weight. ID: 181818168 (sharp) RefCnt: 1 Uptime: 00:00:32 VRF: default Valid, Installed Depends: (22) (24) (28) via 192.168.119.1, enp13s0 (vrf default), weight 182 via 192.168.119.2, enp13s0 (vrf default), weight 218 via 192.168.119.3, enp13s0 (vrf default), weight 255 ID: 181818169 (sharp) RefCnt: 1 Uptime: 00:00:14 VRF: default Valid, Installed Depends: (22) (24) (28) via 192.168.119.1, enp13s0 (vrf default), weight 153 via 192.168.119.2, enp13s0 (vrf default), weight 204 via 192.168.119.3, enp13s0 (vrf default), weight 255 id 22 via 192.168.119.1 dev enp13s0 scope link proto 194 id 24 via 192.168.119.2 dev enp13s0 scope link proto 194 id 28 via 192.168.119.3 dev enp13s0 scope link proto 194 id 181818168 group 22,182/24,218/28,255 proto 194 id 181818169 group 22,153/24,204/28,255 proto 194 Signed-off-by: Donald Sharp --- zebra/zebra_nhg.c | 48 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 8164e2a56fae..96807356d622 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1417,6 +1417,11 @@ static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh, */ nexthop_copy_no_recurse(&lookup, nh, NULL); + /* + * So this is to intentionally cause the singleton nexthop + * to be created with a weight of 1. + */ + lookup.weight = 1; nhe = zebra_nhg_find_nexthop(0, &lookup, afi, type, from_dplane); /* The copy may have allocated labels; free them if necessary. */ @@ -3010,13 +3015,14 @@ int nexthop_active_update(struct route_node *rn, struct route_entry *re) * I'm pretty sure we only allow ONE level of group within group currently. * But making this recursive just in case that ever changes. */ -static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, - uint8_t curr_index, +static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, uint8_t curr_index, struct nhg_hash_entry *nhe, + struct nhg_hash_entry *original, int max_num) { struct nhg_connected *rb_node_dep = NULL; struct nhg_hash_entry *depend = NULL; + struct nexthop *nexthop; uint8_t i = curr_index; frr_each(nhg_connected_tree, &nhe->nhg_depends, rb_node_dep) { @@ -3043,8 +3049,11 @@ static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, if (!zebra_nhg_depends_is_empty(depend)) { /* This is a group within a group */ - i = zebra_nhg_nhe2grp_internal(grp, i, depend, max_num); + i = zebra_nhg_nhe2grp_internal(grp, i, depend, nhe, + max_num); } else { + bool found; + if (!CHECK_FLAG(depend->flags, NEXTHOP_GROUP_VALID)) { if (IS_ZEBRA_DEBUG_RIB_DETAILED || IS_ZEBRA_DEBUG_NHG) @@ -3085,8 +3094,37 @@ static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, continue; } + /* + * So we need to create the nexthop group with + * the appropriate weights. The nexthops weights + * are stored in the fully resolved nexthops for + * the nhg so we need to find the appropriate + * nexthop associated with this and set the weight + * appropriately + */ + found = false; + for (ALL_NEXTHOPS_PTR(&original->nhg, nexthop)) { + if (CHECK_FLAG(nexthop->flags, + NEXTHOP_FLAG_RECURSIVE)) + continue; + + if (nexthop_cmp_no_weight(depend->nhg.nexthop, + nexthop) != 0) + continue; + + found = true; + break; + } + + if (!found) { + if (IS_ZEBRA_DEBUG_RIB_DETAILED || + IS_ZEBRA_DEBUG_NHG) + zlog_debug("%s: Nexthop ID (%u) unable to find nexthop in Nexthop Gropu Entry, something is terribly wrong", + __func__, depend->id); + continue; + } grp[i].id = depend->id; - grp[i].weight = depend->nhg.nexthop->weight; + grp[i].weight = nexthop->weight; i++; } } @@ -3110,7 +3148,7 @@ uint8_t zebra_nhg_nhe2grp(struct nh_grp *grp, struct nhg_hash_entry *nhe, int max_num) { /* Call into the recursive function */ - return zebra_nhg_nhe2grp_internal(grp, 0, nhe, max_num); + return zebra_nhg_nhe2grp_internal(grp, 0, nhe, nhe, max_num); } void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe)