From 46179f6f3e670b2b7566762aeffc2279fe007064 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 16 Jun 2022 14:45:28 -0400 Subject: [PATCH 1/5] bgpd: Use %pSU instead of sockunion2str Commit: 09f267ec95de6d introduced more sockunion2str usages when FRR should be using %pSU. This commit broke the compile when using --enable-dev-build Signed-off-by: Donald Sharp --- bgpd/bgp_bmp.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c index e891f4ba42..25712908df 100644 --- a/bgpd/bgp_bmp.c +++ b/bgpd/bgp_bmp.c @@ -1774,7 +1774,6 @@ static void bmp_active_setup(struct bmp_active *ba); static void bmp_active_connect(struct bmp_active *ba) { enum connect_result res; - char buf[SU_ADDRSTRLEN]; struct interface *ifp; vrf_id_t vrf_id = VRF_DEFAULT; int res_bind; @@ -1799,11 +1798,8 @@ static void bmp_active_connect(struct bmp_active *ba) ba->ifsrc); continue; } - zlog_info("bmp[%s]: selected source address : %s", - ba->ifsrc, - sockunion2str(&ba->addrsrc, - buf, - SU_ADDRSTRLEN)); + zlog_info("bmp[%s]: selected source address : %pSU", + ba->ifsrc, &ba->addrsrc); } ba->socket = sockunion_socket(&ba->addrs[ba->addrpos]); @@ -1819,10 +1815,9 @@ static void bmp_active_connect(struct bmp_active *ba) res_bind = sockunion_bind(ba->socket, &ba->addrsrc, 0, &ba->addrsrc); if (res_bind < 0) { - sockunion2str(&ba->addrsrc, buf, sizeof(buf)); zlog_warn( - "bmp[%s]: no bind currently to source address %s:%d", - ba->hostname, buf, ba->port); + "bmp[%s]: no bind currently to source address %pSU:%d", + ba->hostname, &ba->addrsrc, ba->port); close(ba->socket); ba->socket = -1; sockunion_init(&ba->addrsrc); @@ -1835,25 +1830,22 @@ static void bmp_active_connect(struct bmp_active *ba) htons(ba->port), 0); switch (res) { case connect_error: - sockunion2str(&ba->addrs[ba->addrpos], buf, - sizeof(buf)); - zlog_warn("bmp[%s]: failed to connect to %s:%d", - ba->hostname, buf, ba->port); + zlog_warn("bmp[%s]: failed to connect to %pSU:%d", + ba->hostname, &ba->addrs[ba->addrpos], + ba->port); close(ba->socket); ba->socket = -1; sockunion_init(&ba->addrsrc); continue; case connect_success: - sockunion2str(&ba->addrs[ba->addrpos], buf, - sizeof(buf)); - zlog_info("bmp[%s]: connected to %s:%d", - ba->hostname, buf, ba->port); + zlog_info("bmp[%s]: connected to %pSU:%d", + ba->hostname, &ba->addrs[ba->addrpos], + ba->port); break; case connect_in_progress: - sockunion2str(&ba->addrs[ba->addrpos], buf, - sizeof(buf)); - zlog_warn("bmp[%s]: connect in progress %s:%d", - ba->hostname, buf, ba->port); + zlog_warn("bmp[%s]: connect in progress %pSU:%d", + ba->hostname, &ba->addrs[ba->addrpos], + ba->port); bmp_active_setup(ba); return; } From 5772319ef156deb6afbee8683bb01c1dc352c9bb Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 15 Jun 2022 17:24:39 -0400 Subject: [PATCH 2/5] zebra: Document some data structures better I keep getting confused about nhg_depends and nhg_dependents. So take a second and write them down for the next person. Signed-off-by: Donald Sharp --- zebra/zebra_nhg.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 0863d90a7e..56de336e47 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -79,13 +79,29 @@ struct nhg_hash_entry { uint32_t flags; - /* Dependency tree for other entries. + /* Dependency trees for other entries. * For instance a group with two * nexthops will have two dependencies * pointing to those nhg_hash_entries. * * Using a rb tree here to make lookups * faster with ID's. + * + * nhg_depends the RB tree of entries that this + * group contains. + * + * nhg_dependents the RB tree of entries that + * this group is being used by + * + * NHG id 3 with nexthops id 1/2 + * nhg(3)->nhg_depends has 1 and 2 in the tree + * nhg(3)->nhg_dependents is empty + * + * nhg(1)->nhg_depends is empty + * nhg(1)->nhg_dependents is 3 in the tree + * + * nhg(2)->nhg_depends is empty + * nhg(3)->nhg_dependents is 3 in the tree */ struct nhg_connected_tree_head nhg_depends, nhg_dependents; From 382858d01500cf8f2289db17c7b07639b1714848 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 15 Jun 2022 16:27:07 -0400 Subject: [PATCH 3/5] zebra: Move where zebra marks a nhg as uninstalled in fib Currently the code is marking the nhg as uninstalled but not causing that to flood up to the dependent nhgs: nhg 3 is a group of 1/2 1 -> interface A 2 -> interface B Suppose A goes down, old code would mark nhg 1 as !VALID and !INSTALLED. Suppose B then goes down, old code would mark nhg 2 as !VALID and !INSTALLED But would not mark nhg 3 as !VALID and !INSTALLED (sort of assuming that it would just be cleaned up by NHG refcounts ). I would prefer that the code is pedantic about nhg 3 actually being removed from the system. This code moves the setting of !INSTALLED into zebra_nhg.c where it really belongs. Signed-off-by: Donald Sharp --- zebra/interface.c | 7 ------- zebra/zebra_nhg.c | 6 ++++++ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/zebra/interface.c b/zebra/interface.c index 96e378444d..93ffeb437c 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -184,13 +184,6 @@ static int if_zebra_new_hook(struct interface *ifp) static void if_nhg_dependents_check_valid(struct nhg_hash_entry *nhe) { zebra_nhg_check_valid(nhe); - if (!CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID)) { - /* If we're in shutdown, this interface event needs to clean - * up installed NHGs, so don't clear that flag directly. - */ - if (!zrouter.in_shutdown) - UNSET_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED); - } } static void if_down_nhg_dependents(const struct interface *ifp) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 500f4b0f1b..e27078d138 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1055,6 +1055,12 @@ static void zebra_nhg_set_invalid(struct nhg_hash_entry *nhe) UNSET_FLAG(nhe->flags, NEXTHOP_GROUP_VALID); + /* If we're in shutdown, this interface event needs to clean + * up installed NHGs, so don't clear that flag directly. + */ + if (!zrouter.in_shutdown) + UNSET_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED); + /* Update validity of nexthops depending on it */ frr_each(nhg_connected_tree, &nhe->nhg_dependents, rb_node_dep) zebra_nhg_check_valid(rb_node_dep->nhe); From 35729f38fa5713b923782ca9921c893bb8d3bc25 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 29 Oct 2021 08:16:13 -0400 Subject: [PATCH 4/5] zebra: Add a timer to nexthop group deletion Before deleting nexthop groups, that are installed, from the system, start a timer and hold the nexthop group for that time. Suppose you have this scenario a) create a static route with 1 x ecmp creates a nhg with 1 x ecmp b) create a static route with 2 x ecmp creates a nhg with 2 x ecmp deletes a's nhg c) create a static route with 3 x ecmp creates a nhg with 3 x ecmp deletes b's nhg d) create a different route with 1 x ecmp creates another 1 x ecmp ( since a's ecmp was deleted ) e) create a different route with 2 x ecmp creates another 2 x ecmp ( since b's ecmp was deleted ) If you don't delete the nhg, start a timer, the nhg's used in steps a and b can be reused for steps d and e. This reduces overhead work with zebra <-> kernel interactions and improves the speed of the system. So modify the code to note that an installed nexthop group should be kept around a bit and hopefully reused. Signed-off-by: Donald Sharp --- zebra/zebra_nhg.c | 30 +++++++++++++++++++++++++++--- zebra/zebra_nhg.h | 11 +++++++++++ zebra/zebra_vty.c | 10 +++++++++- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index e27078d138..542972d175 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1625,6 +1625,17 @@ void zebra_nhg_hash_free(void *p) zebra_nhg_free((struct nhg_hash_entry *)p); } +static void zebra_nhg_timer(struct thread *thread) +{ + struct nhg_hash_entry *nhe = THREAD_ARG(thread); + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("Nexthop Timer for nhe: %pNG", nhe); + + if (nhe->refcnt == 1) + zebra_nhg_decrement_ref(nhe); +} + void zebra_nhg_decrement_ref(struct nhg_hash_entry *nhe) { if (IS_ZEBRA_DEBUG_NHG_DETAIL) @@ -1633,6 +1644,15 @@ void zebra_nhg_decrement_ref(struct nhg_hash_entry *nhe) nhe->refcnt--; + if (!zrouter.in_shutdown && nhe->refcnt <= 0 && + CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) && + !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_KEEP_AROUND)) { + nhe->refcnt = 1; + SET_FLAG(nhe->flags, NEXTHOP_GROUP_KEEP_AROUND); + thread_add_timer(zrouter.master, zebra_nhg_timer, nhe, 180, + &nhe->timer); + } + if (!zebra_nhg_depends_is_empty(nhe)) nhg_connected_tree_decrement_ref(&nhe->nhg_depends); @@ -1648,6 +1668,12 @@ void zebra_nhg_increment_ref(struct nhg_hash_entry *nhe) nhe->refcnt++; + if (thread_is_scheduled(nhe->timer)) { + THREAD_OFF(nhe->timer); + nhe->refcnt--; + UNSET_FLAG(nhe->flags, NEXTHOP_GROUP_KEEP_AROUND); + } + if (!zebra_nhg_depends_is_empty(nhe)) nhg_connected_tree_increment_ref(&nhe->nhg_depends); } @@ -3296,9 +3322,6 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, rib_handle_nhg_replace(old, new); - /* if this != 1 at this point, we have a bug */ - assert(old->refcnt == 1); - /* We have to decrement its singletons * because some might not exist in NEW. */ @@ -3310,6 +3333,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, /* Dont call the dec API, we dont want to uninstall the ID */ old->refcnt = 0; + THREAD_OFF(old->timer); zebra_nhg_free(old); old = NULL; } diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 56de336e47..6d2ab248f9 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -105,6 +105,8 @@ struct nhg_hash_entry { */ struct nhg_connected_tree_head nhg_depends, nhg_dependents; + struct thread *timer; + /* * Is this nexthop group valid, ie all nexthops are fully resolved. * What is fully resolved? It's a nexthop that is either self contained @@ -145,6 +147,15 @@ struct nhg_hash_entry { */ #define NEXTHOP_GROUP_PROTO_RELEASED (1 << 5) +/* + * When deleting a NHG notice that it is still installed + * and if it is, slightly delay the actual removal to + * the future. So that upper level protocols might + * be able to take advantage of some NHG's that + * are there + */ +#define NEXTHOP_GROUP_KEEP_AROUND (1 << 6) + /* * Track FPM installation status.. */ diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index be19b07d9d..88752edb9f 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -1433,14 +1433,22 @@ static void show_nexthop_group_out(struct vty *vty, struct nhg_hash_entry *nhe) struct nhg_connected *rb_node_dep = NULL; struct nexthop_group *backup_nhg; char up_str[MONOTIME_STRLEN]; + char time_left[MONOTIME_STRLEN]; uptime2str(nhe->uptime, up_str, sizeof(up_str)); vty_out(vty, "ID: %u (%s)\n", nhe->id, zebra_route_string(nhe->type)); - vty_out(vty, " RefCnt: %u\n", nhe->refcnt); + vty_out(vty, " RefCnt: %u", nhe->refcnt); + if (thread_is_scheduled(nhe->timer)) + vty_out(vty, " Time to Deletion: %s", + thread_timer_to_hhmmss(time_left, sizeof(time_left), + nhe->timer)); + vty_out(vty, "\n"); + vty_out(vty, " Uptime: %s\n", up_str); vty_out(vty, " VRF: %s\n", vrf_id_to_name(nhe->vrf_id)); + if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID)) { vty_out(vty, " Valid"); if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED)) From c9af62e314893d8d3bdb7b3fcd91249e8e401064 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 15 Jun 2022 19:54:29 -0400 Subject: [PATCH 5/5] zebra: Add a configurable knob `zebra nexthop-group keep (1-3600)` Allow end operator to set how long a nexthop-group is kept around in the system after it is no-longer being used. Signed-off-by: Donald Sharp --- doc/user/zebra.rst | 6 ++++++ zebra/zebra_nhg.c | 4 ++-- zebra/zebra_router.c | 2 ++ zebra/zebra_router.h | 3 +++ zebra/zebra_vty.c | 21 +++++++++++++++++++++ 5 files changed, 34 insertions(+), 2 deletions(-) diff --git a/doc/user/zebra.rst b/doc/user/zebra.rst index 29f305520a..eca67c0609 100644 --- a/doc/user/zebra.rst +++ b/doc/user/zebra.rst @@ -273,6 +273,12 @@ Nexthop tracking doesn't resolve nexthops via the default route by default. Allowing this might be useful when e.g. you want to allow BGP to peer across the default route. +.. clicmd:: zebra nexthop-group keep (1-3600) + + Set the time that zebra will keep a created and installed nexthop group + before removing it from the system if the nexthop group is no longer + being used. The default time is 180 seconds. + .. clicmd:: ip nht resolve-via-default Allow IPv4 nexthop tracking to resolve via the default route. This parameter diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 542972d175..f025507f7d 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1649,8 +1649,8 @@ void zebra_nhg_decrement_ref(struct nhg_hash_entry *nhe) !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_KEEP_AROUND)) { nhe->refcnt = 1; SET_FLAG(nhe->flags, NEXTHOP_GROUP_KEEP_AROUND); - thread_add_timer(zrouter.master, zebra_nhg_timer, nhe, 180, - &nhe->timer); + thread_add_timer(zrouter.master, zebra_nhg_timer, nhe, + zrouter.nhg_keep, &nhe->timer); } if (!zebra_nhg_depends_is_empty(nhe)) diff --git a/zebra/zebra_router.c b/zebra/zebra_router.c index 6b4a7543cd..92d519bad1 100644 --- a/zebra/zebra_router.c +++ b/zebra/zebra_router.c @@ -278,6 +278,8 @@ void zebra_router_init(bool asic_offload, bool notify_on_ack) zrouter.packets_to_process = ZEBRA_ZAPI_PACKETS_TO_PROCESS; + zrouter.nhg_keep = ZEBRA_DEFAULT_NHG_KEEP_TIMER; + zebra_vxlan_init(); zebra_mlag_init(); diff --git a/zebra/zebra_router.h b/zebra/zebra_router.h index 7aca91959c..c96c8e5f46 100644 --- a/zebra/zebra_router.h +++ b/zebra/zebra_router.h @@ -219,6 +219,9 @@ struct zebra_router { bool notify_on_ack; bool supports_nhgs; + +#define ZEBRA_DEFAULT_NHG_KEEP_TIMER 180 + uint32_t nhg_keep; }; #define GRACEFUL_RESTART_TIME 60 diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index 88752edb9f..9149da8b0d 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -3850,11 +3850,31 @@ DEFUN (no_ip_zebra_import_table, return (zebra_import_table(AFI_IP, VRF_DEFAULT, table_id, 0, NULL, 0)); } +DEFPY (zebra_nexthop_group_keep, + zebra_nexthop_group_keep_cmd, + "[no] zebra nexthop-group keep (1-3600)", + NO_STR + ZEBRA_STR + "Nexthop-Group\n" + "How long to keep\n" + "Time in seconds from 1-3600\n") +{ + if (no) + zrouter.nhg_keep = ZEBRA_DEFAULT_NHG_KEEP_TIMER; + else + zrouter.nhg_keep = keep; + + return CMD_SUCCESS; +} + static int config_write_protocol(struct vty *vty) { if (allow_delete) vty_out(vty, "allow-external-route-update\n"); + if (zrouter.nhg_keep != ZEBRA_DEFAULT_NHG_KEEP_TIMER) + vty_out(vty, "zebra nexthop-group keep %u\n", zrouter.nhg_keep); + if (zrouter.ribq->spec.hold != ZEBRA_RIB_PROCESS_HOLD_TIME) vty_out(vty, "zebra work-queue %u\n", zrouter.ribq->spec.hold); @@ -4433,6 +4453,7 @@ void zebra_vty_init(void) install_element(CONFIG_NODE, &ip_multicast_mode_cmd); install_element(CONFIG_NODE, &no_ip_multicast_mode_cmd); + install_element(CONFIG_NODE, &zebra_nexthop_group_keep_cmd); install_element(CONFIG_NODE, &ip_zebra_import_table_distance_cmd); install_element(CONFIG_NODE, &no_ip_zebra_import_table_cmd); install_element(CONFIG_NODE, &zebra_workqueue_timer_cmd);