From 4d4c3966c955e6131ea2fd58376da21e5c7256fc Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 11 Mar 2024 15:26:14 -0400 Subject: [PATCH 01/11] tests: Explicitly call out bgp timers for bgp_evpn_mh test Signed-off-by: Donald Sharp --- tests/topotests/bgp_evpn_mh/leaf1/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/leaf2/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/spine1/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/spine2/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/torm11/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/torm12/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/torm21/evpn.conf | 1 + tests/topotests/bgp_evpn_mh/torm22/evpn.conf | 1 + 8 files changed, 8 insertions(+) diff --git a/tests/topotests/bgp_evpn_mh/leaf1/evpn.conf b/tests/topotests/bgp_evpn_mh/leaf1/evpn.conf index 33b6d08aba..d246517898 100644 --- a/tests/topotests/bgp_evpn_mh/leaf1/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/leaf1/evpn.conf @@ -1,6 +1,7 @@ frr defaults datacenter ! router bgp 65101 + timers bgp 3 10 bgp router-id 192.168.100.13 no bgp ebgp-requires-policy neighbor 192.168.50.1 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/leaf2/evpn.conf b/tests/topotests/bgp_evpn_mh/leaf2/evpn.conf index 428998b0fe..6855a436d4 100644 --- a/tests/topotests/bgp_evpn_mh/leaf2/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/leaf2/evpn.conf @@ -1,6 +1,7 @@ frr defaults datacenter ! router bgp 65101 + timers bgp 3 10 bgp router-id 192.168.100.14 no bgp ebgp-requires-policy neighbor 192.168.61.1 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/spine1/evpn.conf b/tests/topotests/bgp_evpn_mh/spine1/evpn.conf index b9fce46ea4..7d6fef699d 100644 --- a/tests/topotests/bgp_evpn_mh/spine1/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/spine1/evpn.conf @@ -1,6 +1,7 @@ frr defaults datacenter ! router bgp 65001 + timers bgp 3 10 bgp router-id 192.168.100.13 no bgp ebgp-requires-policy neighbor 192.168.50.2 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/spine2/evpn.conf b/tests/topotests/bgp_evpn_mh/spine2/evpn.conf index 1430e10b68..c651ada686 100644 --- a/tests/topotests/bgp_evpn_mh/spine2/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/spine2/evpn.conf @@ -1,6 +1,7 @@ frr defaults datacenter ! router bgp 65001 + timers bgp 3 10 bgp router-id 192.168.100.14 no bgp ebgp-requires-policy neighbor 192.168.60.2 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/torm11/evpn.conf b/tests/topotests/bgp_evpn_mh/torm11/evpn.conf index 2c1c695a18..62b7ec5855 100644 --- a/tests/topotests/bgp_evpn_mh/torm11/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/torm11/evpn.conf @@ -7,6 +7,7 @@ frr defaults datacenter ! ! router bgp 65002 + timers bgp 3 10 bgp router-id 192.168.100.15 no bgp ebgp-requires-policy neighbor 192.168.1.1 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/torm12/evpn.conf b/tests/topotests/bgp_evpn_mh/torm12/evpn.conf index 8b0ce1d98f..3ceb974c47 100644 --- a/tests/topotests/bgp_evpn_mh/torm12/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/torm12/evpn.conf @@ -7,6 +7,7 @@ frr defaults datacenter ! ! router bgp 65003 + timers bgp 3 10 bgp router-id 192.168.100.16 no bgp ebgp-requires-policy neighbor 192.168.2.1 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/torm21/evpn.conf b/tests/topotests/bgp_evpn_mh/torm21/evpn.conf index 5247dc1ebd..ecaf85ddb7 100644 --- a/tests/topotests/bgp_evpn_mh/torm21/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/torm21/evpn.conf @@ -7,6 +7,7 @@ frr defaults datacenter ! ! router bgp 65004 + timers bgp 3 10 bgp router-id 192.168.100.17 no bgp ebgp-requires-policy neighbor 192.168.3.1 remote-as external diff --git a/tests/topotests/bgp_evpn_mh/torm22/evpn.conf b/tests/topotests/bgp_evpn_mh/torm22/evpn.conf index ec56360176..c7e152498c 100644 --- a/tests/topotests/bgp_evpn_mh/torm22/evpn.conf +++ b/tests/topotests/bgp_evpn_mh/torm22/evpn.conf @@ -6,6 +6,7 @@ frr defaults datacenter ! debug bgp zebra ! router bgp 65005 + timers bgp 3 10 bgp router-id 192.168.100.18 no bgp ebgp-requires-policy neighbor 192.168.4.1 remote-as external From 829a2e9bc40c4708b3ac4ffb80903ec17f4fb38d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 11 Mar 2024 14:40:49 -0400 Subject: [PATCH 02/11] tests: teste_ospf_rte_calc.py uses bgp add pytest mark Signed-off-by: Donald Sharp --- tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py b/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py index f0950a2db3..45c1325917 100644 --- a/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py +++ b/tests/topotests/ospf_basic_functionality/test_ospf_rte_calc.py @@ -49,7 +49,7 @@ from lib.ospf import ( verify_ospf_interface, ) -pytestmark = [pytest.mark.ospfd, pytest.mark.staticd] +pytestmark = [pytest.mark.bgpd, pytest.mark.ospfd, pytest.mark.staticd] # Global variables From f4fd5c8e36f6f7e6d664b8542abdeada6fdfb46c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 11 Mar 2024 11:22:49 -0400 Subject: [PATCH 03/11] bgpd: Modify update_evpn_type5_route_entry to include path_info pointer Modify update_evpn_type5_route_entry to return a pointer to the struct bgp_path_info modified in this function. This code merely follows the standards used in other bgp_evpn.c code where the update function returns the pointer to the path info. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index a846484f0e..8f9a4dbab2 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -1561,7 +1561,8 @@ static struct bgp_path_info *bgp_evpn_route_get_local_path( static int update_evpn_type5_route_entry(struct bgp *bgp_evpn, struct bgp *bgp_vrf, afi_t afi, safi_t safi, struct bgp_dest *dest, - struct attr *attr, int *route_changed) + struct attr *attr, int *route_changed, + struct bgp_path_info **entry) { struct attr *attr_new = NULL; struct bgp_path_info *pi = NULL; @@ -1599,8 +1600,8 @@ static int update_evpn_type5_route_entry(struct bgp *bgp_evpn, /* add the route entry to route node*/ bgp_path_info_add(dest, pi); + *entry = pi; } else { - tmp_pi = local_pi; if (!attrhash_cmp(tmp_pi->attr, attr)) { @@ -1622,6 +1623,7 @@ static int update_evpn_type5_route_entry(struct bgp *bgp_evpn, tmp_pi->attr = attr_new; tmp_pi->uptime = monotime(NULL); } + *entry = local_pi; } return 0; } @@ -1637,6 +1639,7 @@ static int update_evpn_type5_route(struct bgp *bgp_vrf, struct prefix_evpn *evp, struct bgp_dest *dest = NULL; struct bgp *bgp_evpn = NULL; int route_changed = 0; + struct bgp_path_info *pi = NULL; bgp_evpn = bgp_get_evpn(); if (!bgp_evpn) @@ -1718,7 +1721,7 @@ static int update_evpn_type5_route(struct bgp *bgp_vrf, struct prefix_evpn *evp, /* create or update the route entry within the route node */ update_evpn_type5_route_entry(bgp_evpn, bgp_vrf, afi, safi, dest, &attr, - &route_changed); + &route_changed, &pi); /* schedule for processing and unlock node */ if (route_changed) { From 1389316cf751ecad5bca106d8b54e0bae4737585 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 6 Mar 2024 15:03:31 -0500 Subject: [PATCH 04/11] bgpd: Fix indentation problem in bgp_recalculate_afi_safi_bestpaths This is seriously indented. Let's make it a bit better. Signed-off-by: Donald Sharp --- bgpd/bgpd.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index d98df754ef..052267492b 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1858,17 +1858,19 @@ void bgp_recalculate_afi_safi_bestpaths(struct bgp *bgp, afi_t afi, safi_t safi) for (dest = bgp_table_top(bgp->rib[afi][safi]); dest; dest = bgp_route_next(dest)) { table = bgp_dest_get_bgp_table_info(dest); - if (table != NULL) { - /* Special handling for 2-level routing - * tables. */ - if (safi == SAFI_MPLS_VPN || safi == SAFI_ENCAP - || safi == SAFI_EVPN) { - for (ndest = bgp_table_top(table); ndest; - ndest = bgp_route_next(ndest)) - bgp_process(bgp, ndest, afi, safi); - } else - bgp_process(bgp, dest, afi, safi); - } + + if (!table) + continue; + + /* Special handling for 2-level routing + * tables. */ + if (safi == SAFI_MPLS_VPN || safi == SAFI_ENCAP + || safi == SAFI_EVPN) { + for (ndest = bgp_table_top(table); ndest; + ndest = bgp_route_next(ndest)) + bgp_process(bgp, ndest, afi, safi); + } else + bgp_process(bgp, dest, afi, safi); } } From 19fc4e7999546cfea03c3fe8855e8012d1c0b160 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 1 Mar 2024 09:49:30 -0500 Subject: [PATCH 05/11] bgpd: Add a path_info_flags dumper for bgp Add a debug function to allow developers to dump flags associated with a bgp_path_info in a human readable format. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 51 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 88baa53510..c9b1723254 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -114,6 +114,45 @@ static const struct message bgp_pmsi_tnltype_str[] = { #define VRFID_NONE_STR "-" #define SOFT_RECONFIG_TASK_MAX_PREFIX 25000 +static inline char *bgp_route_dump_path_info_flags(struct bgp_path_info *pi, + char *buf, size_t len) +{ + uint32_t flags = pi->flags; + + if (flags == 0) { + snprintfrr(buf, len, "None "); + return buf; + } + + snprintfrr(buf, len, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s", + CHECK_FLAG(flags, BGP_PATH_IGP_CHANGED) ? "IGP Changed " : "", + CHECK_FLAG(flags, BGP_PATH_DAMPED) ? "Damped" : "", + CHECK_FLAG(flags, BGP_PATH_HISTORY) ? "History " : "", + CHECK_FLAG(flags, BGP_PATH_SELECTED) ? "Selected " : "", + CHECK_FLAG(flags, BGP_PATH_VALID) ? "Valid " : "", + CHECK_FLAG(flags, BGP_PATH_ATTR_CHANGED) ? "Attr Changed " + : "", + CHECK_FLAG(flags, BGP_PATH_DMED_CHECK) ? "Dmed Check " : "", + CHECK_FLAG(flags, BGP_PATH_DMED_SELECTED) ? "Dmed Selected " + : "", + CHECK_FLAG(flags, BGP_PATH_STALE) ? "Stale " : "", + CHECK_FLAG(flags, BGP_PATH_REMOVED) ? "Removed " : "", + CHECK_FLAG(flags, BGP_PATH_COUNTED) ? "Counted " : "", + CHECK_FLAG(flags, BGP_PATH_MULTIPATH) ? "Mpath " : "", + CHECK_FLAG(flags, BGP_PATH_MULTIPATH_CHG) ? "Mpath Chg " : "", + CHECK_FLAG(flags, BGP_PATH_RIB_ATTR_CHG) ? "Rib Chg " : "", + CHECK_FLAG(flags, BGP_PATH_ANNC_NH_SELF) ? "NH Self " : "", + CHECK_FLAG(flags, BGP_PATH_LINK_BW_CHG) ? "LinkBW Chg " : "", + CHECK_FLAG(flags, BGP_PATH_ACCEPT_OWN) ? "Accept Own " : "", + CHECK_FLAG(flags, BGP_PATH_MPLSVPN_LABEL_NH) ? "MPLS Label " + : "", + CHECK_FLAG(flags, BGP_PATH_MPLSVPN_NH_LABEL_BIND) + ? "MPLS Label Bind " + : ""); + + return buf; +} + DEFINE_HOOK(bgp_process, (struct bgp * bgp, afi_t afi, safi_t safi, struct bgp_dest *bn, struct peer *peer, bool withdraw), @@ -679,12 +718,18 @@ int bgp_path_info_cmp(struct bgp *bgp, struct bgp_path_info *new, } if (debug) { + char buf1[256], buf2[256]; + bpi_ultimate = bgp_get_imported_bpi_ultimate(exist); bgp_path_info_path_with_addpath_rx_str(bpi_ultimate, exist_buf, sizeof(exist_buf)); - zlog_debug("%s(%s): Comparing %s flags 0x%x with %s flags 0x%x", - pfx_buf, bgp->name_pretty, new_buf, new->flags, - exist_buf, exist->flags); + zlog_debug("%s(%s): Comparing %s flags %s with %s flags %s", + pfx_buf, bgp->name_pretty, new_buf, + bgp_route_dump_path_info_flags(new, buf1, + sizeof(buf1)), + exist_buf, + bgp_route_dump_path_info_flags(exist, buf2, + sizeof(buf2))); } newattr = new->attr; From 6c8bfaa66e1a1fa2dc223de7406448262f31285e Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 1 Mar 2024 10:01:35 -0500 Subject: [PATCH 06/11] bgpd: Add BGP_PATH_UNSORTED for future commits Add a new flag BGP_PATH_UNSORTED to keep track of sorted -vs- unsorted path_info's. Add some ability to the system to understand when that flag is set. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 36 ++++++++++++------- bgpd/bgp_route.h | 3 +- .../r1/show_bgp_ipv4-post4.1.ref | 2 +- .../r1/show_bgp_ipv4-post5.0.ref | 2 +- .../r1/show_bgp_ipv4-post6.1.ref | 2 +- .../all_protocol_startup/r1/show_bgp_ipv4.ref | 2 +- .../r1/show_bgp_ipv6-post4.1.ref | 2 +- .../all_protocol_startup/r1/show_bgp_ipv6.ref | 2 +- .../r1/show_bgp_ipv6_post6.1.ref | 2 +- 9 files changed, 32 insertions(+), 21 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index c9b1723254..f9e4482317 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -124,7 +124,7 @@ static inline char *bgp_route_dump_path_info_flags(struct bgp_path_info *pi, return buf; } - snprintfrr(buf, len, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s", + snprintfrr(buf, len, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s", CHECK_FLAG(flags, BGP_PATH_IGP_CHANGED) ? "IGP Changed " : "", CHECK_FLAG(flags, BGP_PATH_DAMPED) ? "Damped" : "", CHECK_FLAG(flags, BGP_PATH_HISTORY) ? "History " : "", @@ -148,7 +148,8 @@ static inline char *bgp_route_dump_path_info_flags(struct bgp_path_info *pi, : "", CHECK_FLAG(flags, BGP_PATH_MPLSVPN_NH_LABEL_BIND) ? "MPLS Label Bind " - : ""); + : "", + CHECK_FLAG(flags, BGP_PATH_UNSORTED) ? "Unsorted " : ""); return buf; } @@ -9013,6 +9014,9 @@ static void route_vty_short_status_out(struct vty *vty, if (path->extra && bgp_path_suppressed(path)) json_object_boolean_true_add(json_path, "suppressed"); + if (CHECK_FLAG(path->flags, BGP_PATH_UNSORTED)) + json_object_boolean_true_add(json_path, "unsorted"); + if (CHECK_FLAG(path->flags, BGP_PATH_VALID) && !CHECK_FLAG(path->flags, BGP_PATH_HISTORY)) json_object_boolean_true_add(json_path, "valid"); @@ -9075,6 +9079,8 @@ static void route_vty_short_status_out(struct vty *vty, /* Selected */ if (CHECK_FLAG(path->flags, BGP_PATH_HISTORY)) vty_out(vty, "h"); + else if (CHECK_FLAG(path->flags, BGP_PATH_UNSORTED)) + vty_out(vty, "u"); else if (CHECK_FLAG(path->flags, BGP_PATH_DAMPED)) vty_out(vty, "d"); else if (CHECK_FLAG(path->flags, BGP_PATH_SELECTED)) @@ -13715,21 +13721,23 @@ enum bgp_pcounts { PCOUNT_COUNTED, PCOUNT_BPATH_SELECTED, PCOUNT_PFCNT, /* the figure we display to users */ + PCOUNT_UNSORTED, PCOUNT_MAX, }; static const char *const pcount_strs[] = { - [PCOUNT_ADJ_IN] = "Adj-in", - [PCOUNT_DAMPED] = "Damped", - [PCOUNT_REMOVED] = "Removed", - [PCOUNT_HISTORY] = "History", - [PCOUNT_STALE] = "Stale", - [PCOUNT_VALID] = "Valid", - [PCOUNT_ALL] = "All RIB", - [PCOUNT_COUNTED] = "PfxCt counted", - [PCOUNT_BPATH_SELECTED] = "PfxCt Best Selected", - [PCOUNT_PFCNT] = "Useable", - [PCOUNT_MAX] = NULL, + [PCOUNT_ADJ_IN] = "Adj-in", + [PCOUNT_DAMPED] = "Damped", + [PCOUNT_REMOVED] = "Removed", + [PCOUNT_HISTORY] = "History", + [PCOUNT_STALE] = "Stale", + [PCOUNT_VALID] = "Valid", + [PCOUNT_ALL] = "All RIB", + [PCOUNT_COUNTED] = "PfxCt counted", + [PCOUNT_BPATH_SELECTED] = "PfxCt Best Selected", + [PCOUNT_PFCNT] = "Useable", + [PCOUNT_UNSORTED] = "Unsorted", + [PCOUNT_MAX] = NULL, }; struct peer_pcounts { @@ -13770,6 +13778,8 @@ static void bgp_peer_count_proc(struct bgp_dest *rn, struct peer_pcounts *pc) pc->count[PCOUNT_PFCNT]++; if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) pc->count[PCOUNT_BPATH_SELECTED]++; + if (CHECK_FLAG(pi->flags, BGP_PATH_UNSORTED)) + pc->count[PCOUNT_UNSORTED]++; if (CHECK_FLAG(pi->flags, BGP_PATH_COUNTED)) { pc->count[PCOUNT_COUNTED]++; diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 9be3f063f3..6993d5273d 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -59,7 +59,7 @@ enum bgp_show_adj_route_type { #define BGP_SHOW_SCODE_HEADER \ "Status codes: s suppressed, d damped, " \ - "h history, * valid, > best, = multipath,\n" \ + "h history, u unsorted, * valid, > best, = multipath,\n" \ " i internal, r RIB-failure, S Stale, R Removed\n" #define BGP_SHOW_OCODE_HEADER \ "Origin codes: i - IGP, e - EGP, ? - incomplete\n" @@ -327,6 +327,7 @@ struct bgp_path_info { #define BGP_PATH_ACCEPT_OWN (1 << 16) #define BGP_PATH_MPLSVPN_LABEL_NH (1 << 17) #define BGP_PATH_MPLSVPN_NH_LABEL_BIND (1 << 18) +#define BGP_PATH_UNSORTED (1 << 19) /* BGP route type. This can be static, RIP, OSPF, BGP etc. */ uint8_t type; diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post4.1.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post4.1.ref index 80c727c7e7..eb4e51a074 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post4.1.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post4.1.ref @@ -1,5 +1,5 @@ BGP table version is 1, local router ID is 192.168.0.1, vrf id 0 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post5.0.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post5.0.ref index 6a837f5ae7..8fe3ea41a6 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post5.0.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post5.0.ref @@ -1,5 +1,5 @@ BGP table version is 1, local router ID is 192.168.0.1, vrf id 0 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post6.1.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post6.1.ref index cc3c098518..67b907131c 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post6.1.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4-post6.1.ref @@ -1,6 +1,6 @@ BGP table version is 1, local router ID is 192.168.0.1, vrf id 0 Default local pref 100, local AS 100 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4.ref index 9b6f2f1c94..4f21a5711b 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv4.ref @@ -1,5 +1,5 @@ BGP table version is 1, local router ID is 192.168.0.1 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Origin codes: i - IGP, e - EGP, ? - incomplete diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6-post4.1.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6-post4.1.ref index 5fe04bfa23..69e44e77ed 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6-post4.1.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6-post4.1.ref @@ -1,5 +1,5 @@ BGP table version is 1, local router ID is 192.168.0.1, vrf id 0 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6.ref index c4469d2230..77aab38b0d 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6.ref @@ -1,5 +1,5 @@ BGP table version is 1, local router ID is 192.168.0.1 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Origin codes: i - IGP, e - EGP, ? - incomplete diff --git a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6_post6.1.ref b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6_post6.1.ref index 6ca07ec01d..a99400a6b0 100644 --- a/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6_post6.1.ref +++ b/tests/topotests/all_protocol_startup/r1/show_bgp_ipv6_post6.1.ref @@ -1,6 +1,6 @@ BGP table version is 1, local router ID is 192.168.0.1, vrf id 0 Default local pref 100, local AS 100 -Status codes: s suppressed, d damped, h history, * valid, > best, = multipath, +Status codes: s suppressed, d damped, h history, u unsorted, * valid, > best, = multipath, i internal, r RIB-failure, S Stale, R Removed Nexthop codes: @NNN nexthop's vrf id, < announce-nh-self Origin codes: i - IGP, e - EGP, ? - incomplete From 04936ab84a2ded92b7813c5a967c007442c4e8b4 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 18 Mar 2024 09:33:21 -0400 Subject: [PATCH 07/11] bgpd: Call bgp_process when bgp_path_info_delete is called bgp_damp.c has an instance of bgp_path_info_delete is called. Thus setting up the path info for deletion, but since it never calls bgp_process, it can never be deleted. This means that in a dampening situation, after a withdrawal the path_info would stick around. Schedule the path for deletion. Signed-off-by: Donald Sharp --- bgpd/bgp_damp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_damp.c b/bgpd/bgp_damp.c index 80425ebe7a..7893884fa6 100644 --- a/bgpd/bgp_damp.c +++ b/bgpd/bgp_damp.c @@ -306,8 +306,10 @@ void bgp_damp_info_free(struct bgp_damp_info *bdi, int withdraw, afi_t afi, bgp_path_info_unset_flag(bdi->dest, path, BGP_PATH_HISTORY | BGP_PATH_DAMPED); - if (bdi->lastrecord == BGP_RECORD_WITHDRAW && withdraw) + if (bdi->lastrecord == BGP_RECORD_WITHDRAW && withdraw) { bgp_path_info_delete(bdi->dest, path); + bgp_process(path->peer->bgp, bdi->dest, afi, safi); + } XFREE(MTYPE_BGP_DAMP_INFO, bdi); } From ab49fc9c48192d231c779068a7a68403850a8edb Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 4 Mar 2024 10:41:13 -0500 Subject: [PATCH 08/11] bgpd: Add pi to bgp_process This will allow a consistency of approach to adding/removing pi's to from the workqueue for processing as well as properly handling the dest->info pi list more appropriately. Signed-off-by: Donald Sharp --- bgpd/bgp_damp.c | 4 +-- bgpd/bgp_evpn.c | 24 +++++++-------- bgpd/bgp_evpn_mh.c | 8 ++--- bgpd/bgp_label.c | 2 +- bgpd/bgp_mplsvpn.c | 14 ++++----- bgpd/bgp_nht.c | 2 +- bgpd/bgp_route.c | 77 ++++++++++++++++++++-------------------------- bgpd/bgp_route.h | 3 +- bgpd/bgp_vty.c | 9 ++++-- bgpd/bgp_zebra.c | 2 +- bgpd/bgpd.c | 16 +++++++--- bgpd/rfapi/rfapi.c | 6 ++-- 12 files changed, 85 insertions(+), 82 deletions(-) diff --git a/bgpd/bgp_damp.c b/bgpd/bgp_damp.c index 7893884fa6..6b6387b1b5 100644 --- a/bgpd/bgp_damp.c +++ b/bgpd/bgp_damp.c @@ -150,7 +150,7 @@ static void bgp_reuse_timer(struct event *t) bgp_aggregate_increment( bgp, bgp_dest_get_prefix(bdi->dest), bdi->path, bdi->afi, bdi->safi); - bgp_process(bgp, bdi->dest, bdi->afi, + bgp_process(bgp, bdi->dest, bdi->path, bdi->afi, bdi->safi); } @@ -308,7 +308,7 @@ void bgp_damp_info_free(struct bgp_damp_info *bdi, int withdraw, afi_t afi, if (bdi->lastrecord == BGP_RECORD_WITHDRAW && withdraw) { bgp_path_info_delete(bdi->dest, path); - bgp_process(path->peer->bgp, bdi->dest, afi, safi); + bgp_process(path->peer->bgp, bdi->dest, path, afi, safi); } XFREE(MTYPE_BGP_DAMP_INFO, bdi); diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index 8f9a4dbab2..fc077ebd17 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -1424,7 +1424,7 @@ static void evpn_delete_old_local_route(struct bgp *bgp, struct bgpevpn *vpn, * this table. */ if (pi) - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, pi, afi, safi); bgp_dest_unlock_node(global_dest); } @@ -1725,7 +1725,7 @@ static int update_evpn_type5_route(struct bgp *bgp_vrf, struct prefix_evpn *evp, /* schedule for processing and unlock node */ if (route_changed) { - bgp_process(bgp_evpn, dest, afi, safi); + bgp_process(bgp_evpn, dest, pi, afi, safi); bgp_dest_unlock_node(dest); } @@ -2273,7 +2273,7 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, false /* setup_sync */, NULL /* old_is_sync */); /* Schedule for processing and unlock node. */ - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, global_pi, afi, safi); bgp_dest_unlock_node(dest); } @@ -2333,7 +2333,7 @@ static int delete_evpn_type5_route(struct bgp *bgp_vrf, struct prefix_evpn *evp) delete_evpn_route_entry(bgp_evpn, afi, safi, dest, &pi); if (pi) - bgp_process(bgp_evpn, dest, afi, safi); + bgp_process(bgp_evpn, dest, pi, afi, safi); bgp_dest_unlock_node(dest); return 0; } @@ -2373,7 +2373,7 @@ static int delete_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, * this table. */ if (pi) - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, pi, afi, safi); bgp_dest_unlock_node(global_dest); } @@ -2536,7 +2536,7 @@ void bgp_evpn_update_type2_route_entry(struct bgp *bgp, struct bgpevpn *vpn, NULL /* old_is_sync */); /* Schedule for processing and unlock node. */ - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, global_pi, afi, safi); bgp_dest_unlock_node(global_dest); } @@ -2621,7 +2621,7 @@ static void delete_global_type2_routes(struct bgp *bgp, struct bgpevpn *vpn) delete_evpn_route_entry(bgp, afi, safi, dest, &pi); if (pi) - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); } /* Unlock RD node. */ @@ -3075,7 +3075,7 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, safi); /* Perform route selection and update zebra, if required. */ - bgp_process(bgp_vrf, dest, afi, safi); + bgp_process(bgp_vrf, dest, pi, afi, safi); /* Process for route leaking. */ vpn_leak_from_vrf_update(bgp_get_default(), bgp_vrf, pi); @@ -3437,7 +3437,7 @@ static int uninstall_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, bgp_evpn_path_nh_del(bgp_vrf, pi); /* Perform route selection and update zebra, if required. */ - bgp_process(bgp_vrf, dest, afi, safi); + bgp_process(bgp_vrf, dest, pi, afi, safi); /* Unlock route node. */ bgp_dest_unlock_node(dest); @@ -4434,7 +4434,7 @@ static void update_advertise_vni_route(struct bgp *bgp, struct bgpevpn *vpn, } /* Schedule for processing and unlock node. */ - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, global_pi, afi, safi); bgp_dest_unlock_node(global_dest); } @@ -4484,7 +4484,7 @@ static void update_advertise_vni_routes(struct bgp *bgp, struct bgpevpn *vpn) false /* setup_sync */, NULL /* old_is_sync */); /* Schedule for processing and unlock node. */ - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, pi, afi, safi); bgp_dest_unlock_node(global_dest); } @@ -4529,7 +4529,7 @@ static int delete_withdraw_vni_routes(struct bgp *bgp, struct bgpevpn *vpn) * this table. */ if (pi) - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, pi, afi, safi); bgp_dest_unlock_node(global_dest); } diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index d88c52d1f6..9347adea76 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -511,7 +511,7 @@ static int bgp_evpn_mh_route_delete(struct bgp *bgp, struct bgp_evpn_es *es, * this table. */ if (pi) - bgp_process(bgp, global_dest, afi, safi); + bgp_process(bgp, global_dest, pi, afi, safi); bgp_dest_unlock_node(global_dest); } @@ -562,7 +562,7 @@ int delete_global_ead_evi_routes(struct bgp *bgp, struct bgpevpn *vpn) delete_evpn_route_entry(bgp, afi, safi, bd, &pi); if (pi) - bgp_process(bgp, bd, afi, safi); + bgp_process(bgp, bd, pi, afi, safi); } } @@ -686,7 +686,7 @@ static int bgp_evpn_type4_route_update(struct bgp *bgp, attr_new, &global_pi, &route_changed); /* Schedule for processing and unlock node. */ - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, global_pi, afi, safi); bgp_dest_unlock_node(dest); } @@ -1025,7 +1025,7 @@ static int bgp_evpn_type1_route_update(struct bgp *bgp, struct bgp_evpn_es *es, attr_new, &global_pi, &route_changed); /* Schedule for processing and unlock node. */ - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, global_pi, afi, safi); bgp_dest_unlock_node(dest); } diff --git a/bgpd/bgp_label.c b/bgpd/bgp_label.c index 7327ab5182..68104967b2 100644 --- a/bgpd/bgp_label.c +++ b/bgpd/bgp_label.c @@ -74,7 +74,7 @@ int bgp_parse_fec_update(void) bgp_set_valid_label(&dest->local_label); } SET_FLAG(dest->flags, BGP_NODE_LABEL_CHANGED); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, NULL, afi, safi); bgp_dest_unlock_node(dest); return 1; } diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 985af84ea9..06b1f52246 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1208,7 +1208,7 @@ leak_update(struct bgp *to_bgp, struct bgp_dest *bn, /* Process change. */ bgp_aggregate_increment(to_bgp, p, bpi, afi, safi); - bgp_process(to_bgp, bn, afi, safi); + bgp_process(to_bgp, bn, bpi, afi, safi); if (debug) zlog_debug("%s: ->%s: %pBD Found route, changed attr", @@ -1270,7 +1270,7 @@ leak_update(struct bgp *to_bgp, struct bgp_dest *bn, bgp_aggregate_increment(to_bgp, p, new, afi, safi); bgp_path_info_add(bn, new); - bgp_process(to_bgp, bn, afi, safi); + bgp_process(to_bgp, bn, new, afi, safi); if (debug) zlog_debug("%s: ->%s: %pBD: Added new route", __func__, @@ -1956,7 +1956,7 @@ void vpn_leak_from_vrf_withdraw(struct bgp *to_bgp, /* to */ bgp_aggregate_decrement(to_bgp, p, bpi, afi, safi); bgp_path_info_delete(bn, bpi); - bgp_process(to_bgp, bn, afi, safi); + bgp_process(to_bgp, bn, bpi, afi, safi); } bgp_dest_unlock_node(bn); } @@ -2012,7 +2012,7 @@ void vpn_leak_from_vrf_withdraw_all(struct bgp *to_bgp, struct bgp *from_bgp, to_bgp, bgp_dest_get_prefix(bn), bpi, afi, safi); bgp_path_info_delete(bn, bpi); - bgp_process(to_bgp, bn, afi, safi); + bgp_process(to_bgp, bn, bpi, afi, safi); bgp_mplsvpn_path_nh_label_unlink( bpi->extra->vrfleak->parent); } @@ -2506,7 +2506,7 @@ void vpn_leak_to_vrf_withdraw(struct bgp_path_info *path_vpn) bpi); bgp_aggregate_decrement(bgp, p, bpi, afi, safi); bgp_path_info_delete(bn, bpi); - bgp_process(bgp, bn, afi, safi); + bgp_process(bgp, bn, bpi, afi, safi); } bgp_dest_unlock_node(bn); } @@ -2538,7 +2538,7 @@ void vpn_leak_to_vrf_withdraw_all(struct bgp *to_bgp, afi_t afi) bgp_dest_get_prefix(bn), bpi, afi, safi); bgp_path_info_delete(bn, bpi); - bgp_process(to_bgp, bn, afi, safi); + bgp_process(to_bgp, bn, bpi, afi, safi); } } } @@ -4189,7 +4189,7 @@ static int bgp_mplsvpn_nh_label_bind_get_local_label_cb(mpls_label_t label, if (!table) continue; SET_FLAG(pi->net->flags, BGP_NODE_LABEL_CHANGED); - bgp_process(table->bgp, pi->net, table->afi, table->safi); + bgp_process(table->bgp, pi->net, pi, table->afi, table->safi); } return 0; diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 786d07c5a9..8d7c089284 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -1432,7 +1432,7 @@ void evaluate_paths(struct bgp_nexthop_cache *bnc) } } - bgp_process(bgp_path, dest, afi, safi); + bgp_process(bgp_path, dest, path, afi, safi); } if (peer) { diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index f9e4482317..661d20bc94 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2882,6 +2882,7 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest, (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { enum bgp_path_selection_reason reason; + UNSET_FLAG(pi->flags, BGP_PATH_UNSORTED); if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) old_select = pi; @@ -3733,13 +3734,22 @@ static struct bgp_process_queue *bgp_processq_alloc(struct bgp *bgp) return pqnode; } -void bgp_process(struct bgp *bgp, struct bgp_dest *dest, afi_t afi, safi_t safi) +void bgp_process(struct bgp *bgp, struct bgp_dest *dest, + struct bgp_path_info *pi, afi_t afi, safi_t safi) { #define ARBITRARY_PROCESS_QLEN 10000 struct work_queue *wq = bgp->process_queue; struct bgp_process_queue *pqnode; int pqnode_reuse = 0; + /* + * Indicate that *this* pi is in an unsorted + * situation, even if the node is already + * scheduled. + */ + if (pi) + SET_FLAG(pi->flags, BGP_PATH_UNSORTED); + /* already scheduled for processing? */ if (CHECK_FLAG(dest->flags, BGP_NODE_PROCESS_SCHEDULED)) return; @@ -3988,7 +3998,7 @@ void bgp_rib_remove(struct bgp_dest *dest, struct bgp_path_info *pi, } hook_call(bgp_process, peer->bgp, afi, safi, dest, peer, true); - bgp_process(peer->bgp, dest, afi, safi); + bgp_process(peer->bgp, dest, pi, afi, safi); } static void bgp_rib_withdraw(struct bgp_dest *dest, struct bgp_path_info *pi, @@ -4593,7 +4603,7 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, != BGP_DAMP_SUPPRESSED) { bgp_aggregate_increment(bgp, p, pi, afi, safi); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); } } else /* Duplicate - odd */ { @@ -4621,7 +4631,7 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, bgp_path_info_unset_flag( dest, pi, BGP_PATH_STALE); bgp_dest_set_defer_flag(dest, false); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); } } @@ -4911,7 +4921,7 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, /* Process change. */ bgp_aggregate_increment(bgp, p, pi, afi, safi); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); bgp_dest_unlock_node(dest); if (SAFI_UNICAST == safi @@ -5056,7 +5066,7 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, hook_call(bgp_process, bgp, afi, safi, dest, peer, false); /* Process change. */ - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, new, afi, safi); if (SAFI_UNICAST == safi && (bgp->inst_type == BGP_INSTANCE_TYPE_VRF @@ -6563,7 +6573,7 @@ void bgp_static_update(struct bgp *bgp, const struct prefix *p, /* Process change. */ bgp_aggregate_increment(bgp, p, pi, afi, safi); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); if (SAFI_MPLS_VPN == safi && bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT) { @@ -6621,7 +6631,7 @@ void bgp_static_update(struct bgp *bgp, const struct prefix *p, bgp_dest_unlock_node(dest); /* Process change. */ - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, new, afi, safi); if (SAFI_UNICAST == safi && (bgp->inst_type == BGP_INSTANCE_TYPE_VRF || @@ -6678,7 +6688,7 @@ void bgp_static_withdraw(struct bgp *bgp, const struct prefix *p, afi_t afi, bgp_aggregate_decrement(bgp, p, pi, afi, safi); bgp_unlink_nexthop(pi); bgp_path_info_delete(dest, pi); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); } /* Unlock bgp_node_lookup. */ @@ -7079,7 +7089,7 @@ static void bgp_purge_af_static_redist_routes(struct bgp *bgp, afi_t afi, safi); bgp_unlink_nexthop(pi); bgp_path_info_delete(dest, pi); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); } } } @@ -7468,7 +7478,7 @@ static void bgp_aggregate_install( SET_FLAG(new->flags, BGP_PATH_VALID); bgp_path_info_add(dest, new); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, new, afi, safi); } else { uninstall_aggregate_route: for (pi = orig; pi; pi = pi->next) @@ -7480,7 +7490,7 @@ static void bgp_aggregate_install( /* Withdraw static BGP route from routing table. */ if (pi) { bgp_path_info_delete(dest, pi); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, pi, afi, safi); } } @@ -7566,7 +7576,6 @@ void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, const struct prefix *dest_p; struct bgp_dest *dest, *top; struct bgp_path_info *pi; - bool toggle_suppression; /* We've found a different MED we must revert any suppressed routes. */ top = bgp_node_get(table, p); @@ -7576,7 +7585,6 @@ void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, if (dest_p->prefixlen <= p->prefixlen) continue; - toggle_suppression = false; for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { if (BGP_PATH_HOLDDOWN(pi)) continue; @@ -7587,17 +7595,14 @@ void bgp_aggregate_toggle_suppressed(struct bgp_aggregate *aggregate, if (suppress) { /* Suppress route if not suppressed already. */ if (aggr_suppress_path(aggregate, pi)) - toggle_suppression = true; + bgp_process(bgp, dest, pi, afi, safi); continue; } /* Install route if there is no more suppression. */ if (aggr_unsuppress_path(aggregate, pi)) - toggle_suppression = true; + bgp_process(bgp, dest, pi, afi, safi); } - - if (toggle_suppression) - bgp_process(bgp, dest, afi, safi); } bgp_dest_unlock_node(top); } @@ -7656,7 +7661,6 @@ bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, struct ecommunity *ecommunity = NULL; struct lcommunity *lcommunity = NULL; struct bgp_path_info *pi; - unsigned long match = 0; uint8_t atomic_aggregate = 0; /* If the bgp instance is being deleted or self peer is deleted @@ -7706,8 +7710,6 @@ bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, if (!bgp_check_advertise(bgp, dest, safi)) continue; - match = 0; - for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { if (BGP_PATH_HOLDDOWN(pi)) continue; @@ -7731,7 +7733,7 @@ bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, if (aggregate->summary_only && AGGREGATE_MED_VALID(aggregate)) { if (aggr_suppress_path(aggregate, pi)) - match++; + bgp_process(bgp, dest, pi, afi, safi); } /* @@ -7747,7 +7749,7 @@ bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, && AGGREGATE_MED_VALID(aggregate) && aggr_suppress_map_test(bgp, aggregate, pi)) { if (aggr_suppress_path(aggregate, pi)) - match++; + bgp_process(bgp, dest, pi, afi, safi); } aggregate->count++; @@ -7808,8 +7810,6 @@ bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, aggregate, bgp_attr_get_lcommunity(pi->attr)); } - if (match) - bgp_process(bgp, dest, afi, safi); } if (aggregate->as_set) { bgp_compute_aggregate_aspath_val(aggregate); @@ -7869,7 +7869,6 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, struct bgp_dest *top; struct bgp_dest *dest; struct bgp_path_info *pi; - unsigned long match; table = bgp->rib[afi][safi]; @@ -7881,7 +7880,6 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, if (dest_p->prefixlen <= p->prefixlen) continue; - match = 0; for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { if (BGP_PATH_HOLDDOWN(pi)) @@ -7899,7 +7897,7 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, if (pi->extra && pi->extra->aggr_suppressors && listcount(pi->extra->aggr_suppressors)) { if (aggr_unsuppress_path(aggregate, pi)) - match++; + bgp_process(bgp, dest, pi, afi, safi); } aggregate->count--; @@ -7941,10 +7939,6 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, pi->attr)); } } - - /* If this node was suppressed, process the change. */ - if (match) - bgp_process(bgp, dest, afi, safi); } if (aggregate->as_set) { aspath_free(aggregate->aspath); @@ -8093,7 +8087,6 @@ static void bgp_remove_route_from_aggregate(struct bgp *bgp, afi_t afi, struct community *community = NULL; struct ecommunity *ecommunity = NULL; struct lcommunity *lcommunity = NULL; - unsigned long match = 0; /* If the bgp instance is being deleted or self peer is deleted * then do not create aggregate route @@ -8110,12 +8103,12 @@ static void bgp_remove_route_from_aggregate(struct bgp *bgp, afi_t afi, if (aggregate->summary_only && AGGREGATE_MED_VALID(aggregate)) if (aggr_unsuppress_path(aggregate, pi)) - match++; + bgp_process(bgp, pi->net, pi, afi, safi); if (aggregate->suppress_map_name && AGGREGATE_MED_VALID(aggregate) && aggr_suppress_map_test(bgp, aggregate, pi)) if (aggr_unsuppress_path(aggregate, pi)) - match++; + bgp_process(bgp, pi->net, pi, afi, safi); /* * This must be called after `summary`, `suppress-map` check to avoid @@ -8157,10 +8150,6 @@ static void bgp_remove_route_from_aggregate(struct bgp *bgp, afi_t afi, aggregate, bgp_attr_get_lcommunity(pi->attr)); } - /* If this node was suppressed, process the change. */ - if (match) - bgp_process(bgp, pi->net, afi, safi); - origin = BGP_ORIGIN_IGP; if (aggregate->incomplete_origin_count > 0) origin = BGP_ORIGIN_INCOMPLETE; @@ -8770,7 +8759,7 @@ void bgp_redistribute_add(struct bgp *bgp, struct prefix *p, /* Process change. */ bgp_aggregate_increment(bgp, p, bpi, afi, SAFI_UNICAST); - bgp_process(bgp, bn, afi, SAFI_UNICAST); + bgp_process(bgp, bn, bpi, afi, SAFI_UNICAST); bgp_dest_unlock_node(bn); aspath_unintern(&attr.aspath); @@ -8793,7 +8782,7 @@ void bgp_redistribute_add(struct bgp *bgp, struct prefix *p, bgp_path_info_add(bn, new); bgp_dest_unlock_node(bn); SET_FLAG(bn->flags, BGP_NODE_FIB_INSTALLED); - bgp_process(bgp, bn, afi, SAFI_UNICAST); + bgp_process(bgp, bn, new, afi, SAFI_UNICAST); if ((bgp->inst_type == BGP_INSTANCE_TYPE_VRF) || (bgp->inst_type == BGP_INSTANCE_TYPE_DEFAULT)) { @@ -8834,7 +8823,7 @@ void bgp_redistribute_delete(struct bgp *bgp, struct prefix *p, uint8_t type, } bgp_aggregate_decrement(bgp, p, pi, afi, SAFI_UNICAST); bgp_path_info_delete(dest, pi); - bgp_process(bgp, dest, afi, SAFI_UNICAST); + bgp_process(bgp, dest, pi, afi, SAFI_UNICAST); } bgp_dest_unlock_node(dest); } @@ -8868,7 +8857,7 @@ void bgp_redistribute_withdraw(struct bgp *bgp, afi_t afi, int type, bgp_path_info_delete(dest, pi); if (!CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS)) - bgp_process(bgp, dest, afi, SAFI_UNICAST); + bgp_process(bgp, dest, pi, afi, SAFI_UNICAST); else { dest = bgp_path_info_reap(dest, pi); assert(dest); diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 6993d5273d..e7dac0ff29 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -802,7 +802,8 @@ extern void bgp_withdraw(struct peer *peer, const struct prefix *p, struct bgp_route_evpn *evpn); /* for bgp_nexthop and bgp_damp */ -extern void bgp_process(struct bgp *, struct bgp_dest *, afi_t, safi_t); +extern void bgp_process(struct bgp *bgp, struct bgp_dest *dest, + struct bgp_path_info *pi, afi_t afi, safi_t safi); /* * Add an end-of-initial-update marker to the process queue. This is just a diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 0a1cf3362b..3496686ad4 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -10738,7 +10738,10 @@ static int bgp_clear_prefix(struct vty *vty, const char *view_name, if (rm_p->prefixlen == match.prefixlen) { SET_FLAG(rm->flags, BGP_NODE_USER_CLEAR); - bgp_process(bgp, rm, afi, safi); + bgp_process(bgp, rm, + bgp_dest_get_bgp_path_info( + rm), + afi, safi); } bgp_dest_unlock_node(rm); } @@ -10750,7 +10753,9 @@ static int bgp_clear_prefix(struct vty *vty, const char *view_name, if (dest_p->prefixlen == match.prefixlen) { SET_FLAG(dest->flags, BGP_NODE_USER_CLEAR); - bgp_process(bgp, dest, afi, safi); + bgp_process(bgp, dest, + bgp_dest_get_bgp_path_info(dest), + afi, safi); } bgp_dest_unlock_node(dest); } diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index 04d520a923..87fdfd5a82 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -2116,7 +2116,7 @@ bool bgp_redistribute_metric_set(struct bgp *bgp, struct bgp_redist *red, bgp_path_info_set_flag(dest, pi, BGP_PATH_ATTR_CHANGED); - bgp_process(bgp, dest, afi, SAFI_UNICAST); + bgp_process(bgp, dest, pi, afi, SAFI_UNICAST); } } } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 052267492b..a4e3bd15ea 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1853,6 +1853,7 @@ void bgp_peer_conf_if_to_su_update(struct peer_connection *connection) void bgp_recalculate_afi_safi_bestpaths(struct bgp *bgp, afi_t afi, safi_t safi) { struct bgp_dest *dest, *ndest; + struct bgp_path_info *pi, *next; struct bgp_table *table; for (dest = bgp_table_top(bgp->rib[afi][safi]); dest; @@ -1867,10 +1868,17 @@ void bgp_recalculate_afi_safi_bestpaths(struct bgp *bgp, afi_t afi, safi_t safi) if (safi == SAFI_MPLS_VPN || safi == SAFI_ENCAP || safi == SAFI_EVPN) { for (ndest = bgp_table_top(table); ndest; - ndest = bgp_route_next(ndest)) - bgp_process(bgp, ndest, afi, safi); - } else - bgp_process(bgp, dest, afi, safi); + ndest = bgp_route_next(ndest)) { + for (pi = bgp_dest_get_bgp_path_info(ndest); + (pi != NULL) && (next = pi->next, 1); + pi = next) + bgp_process(bgp, ndest, pi, afi, safi); + } + } else { + for (pi = bgp_dest_get_bgp_path_info(dest); + (pi != NULL) && (next = pi->next, 1); pi = next) + bgp_process(bgp, dest, pi, afi, safi); + } } } diff --git a/bgpd/rfapi/rfapi.c b/bgpd/rfapi/rfapi.c index 382af05c24..ae899daf82 100644 --- a/bgpd/rfapi/rfapi.c +++ b/bgpd/rfapi/rfapi.c @@ -463,7 +463,7 @@ void del_vnc_route(struct rfapi_descriptor *rfd, bgp_aggregate_decrement(bgp, p, bpi, afi, safi); bgp_path_info_delete(bn, bpi); - bgp_process(bgp, bn, afi, safi); + bgp_process(bgp, bn, bpi, afi, safi); } else { vnc_zlog_debug_verbose( "%s: Couldn't find route (safi=%d) at prefix %pFX", @@ -1001,7 +1001,7 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */ /* Process change. */ bgp_aggregate_increment(bgp, p, bpi, afi, safi); - bgp_process(bgp, bn, afi, safi); + bgp_process(bgp, bn, bpi, afi, safi); bgp_dest_unlock_node(bn); vnc_zlog_debug_any( @@ -1046,7 +1046,7 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */ } bgp_dest_unlock_node(bn); - bgp_process(bgp, bn, afi, safi); + bgp_process(bgp, bn, new, afi, safi); vnc_zlog_debug_any( "%s: Added route (safi=%s) at prefix %s (bn=%p, prd=%pRDP)", From fca805972d58e2ddfa16916932447ff7f9e9d61d Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 11 Mar 2024 14:05:59 -0400 Subject: [PATCH 09/11] bgpd: bgp_best_selection is inherently pi based Currently evpn code calls bgp_best_selection for local decisions for local tables to figure out what to do. This is also pi based so let's note that the pi has been changed before calling bgp_best_selection. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 14 ++++++++------ bgpd/bgp_evpn_mh.c | 13 ++++++++----- bgpd/bgp_evpn_private.h | 3 ++- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index fc077ebd17..5f3f0f923c 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -1438,7 +1438,7 @@ static void evpn_delete_old_local_route(struct bgp *bgp, struct bgpevpn *vpn, * Note: vpn is NULL for local EAD-ES routes. */ int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn, - struct bgp_dest *dest) + struct bgp_dest *dest, struct bgp_path_info *pi) { struct bgp_path_info *old_select, *new_select; struct bgp_path_info_pair old_and_new; @@ -1446,6 +1446,8 @@ int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn, safi_t safi = SAFI_EVPN; int ret = 0; + SET_FLAG(pi->flags, BGP_PATH_UNSORTED); + /* Compute the best path. */ bgp_best_selection(bgp, dest, &bgp->maxpaths[afi][safi], &old_and_new, afi, safi); @@ -2225,7 +2227,7 @@ static int update_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, * route would win, and we should evict the defunct local route * and (re)install the remote route into zebra. */ - evpn_route_select_install(bgp, vpn, dest); + evpn_route_select_install(bgp, vpn, dest, pi); /* * If the new local route was not selected evict it and tell zebra * to re-add the best remote dest. BGP doesn't retain non-best local @@ -2383,7 +2385,7 @@ static int delete_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, if (pi) { dest = bgp_path_info_reap(dest, pi); assert(dest); - evpn_route_select_install(bgp, vpn, dest); + evpn_route_select_install(bgp, vpn, dest, pi); } /* dest should still exist due to locking make coverity happy */ @@ -2497,7 +2499,7 @@ void bgp_evpn_update_type2_route_entry(struct bgp *bgp, struct bgpevpn *vpn, * advertised to peers; otherwise, ensure it is evicted and * (re)install the remote route into zebra. */ - evpn_route_select_install(bgp, vpn, dest); + evpn_route_select_install(bgp, vpn, dest, pi); if (CHECK_FLAG(pi->flags, BGP_PATH_REMOVED)) { route_change = 0; @@ -3187,7 +3189,7 @@ static int install_evpn_route_entry_in_vni_common( bgp_evpn_remote_ip_hash_add(vpn, pi); /* Perform route selection and update zebra, if required. */ - ret = evpn_route_select_install(bgp, vpn, dest); + ret = evpn_route_select_install(bgp, vpn, dest, pi); /* if the best path is a local path with a non-zero ES * sync info against the local path may need to be updated @@ -3229,7 +3231,7 @@ static int uninstall_evpn_route_entry_in_vni_common( bgp_path_info_delete(dest, pi); /* Perform route selection and update zebra, if required. */ - ret = evpn_route_select_install(bgp, vpn, dest); + ret = evpn_route_select_install(bgp, vpn, dest, pi); /* if the best path is a local path with a non-zero ES * sync info against the local path may need to be updated diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index 9347adea76..2e148c0003 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -91,7 +91,8 @@ static void bgp_evpn_path_nh_unlink(struct bgp_path_evpn_nh_info *nh_info); */ static int bgp_evpn_es_route_select_install(struct bgp *bgp, struct bgp_evpn_es *es, - struct bgp_dest *dest) + struct bgp_dest *dest, + struct bgp_path_info *pi) { int ret = 0; afi_t afi = AFI_L2VPN; @@ -100,6 +101,8 @@ static int bgp_evpn_es_route_select_install(struct bgp *bgp, struct bgp_path_info *new_select; /* new best */ struct bgp_path_info_pair old_and_new; + SET_FLAG(pi->flags, BGP_PATH_UNSORTED); + /* Compute the best path. */ bgp_best_selection(bgp, dest, &bgp->maxpaths[afi][safi], &old_and_new, afi, safi); @@ -231,7 +234,7 @@ static int bgp_evpn_es_route_install(struct bgp *bgp, } /* Perform route selection and update zebra, if required. */ - ret = bgp_evpn_es_route_select_install(bgp, es, dest); + ret = bgp_evpn_es_route_select_install(bgp, es, dest, pi); bgp_dest_unlock_node(dest); @@ -272,7 +275,7 @@ static int bgp_evpn_es_route_uninstall(struct bgp *bgp, struct bgp_evpn_es *es, bgp_path_info_delete(dest, pi); /* Perform route selection and update zebra, if required. */ - ret = bgp_evpn_es_route_select_install(bgp, es, dest); + ret = bgp_evpn_es_route_select_install(bgp, es, dest, pi); /* Unlock route node. */ bgp_dest_unlock_node(dest); @@ -668,7 +671,7 @@ static int bgp_evpn_type4_route_update(struct bgp *bgp, * this is just to set the flags correctly * as local route in the ES always wins. */ - bgp_evpn_es_route_select_install(bgp, es, dest); + bgp_evpn_es_route_select_install(bgp, es, dest, pi); bgp_dest_unlock_node(dest); /* If this is a new route or some attribute has changed, export the @@ -1008,7 +1011,7 @@ static int bgp_evpn_type1_route_update(struct bgp *bgp, struct bgp_evpn_es *es, * this is just to set the flags correctly as local route in * the ES always wins. */ - evpn_route_select_install(bgp, vpn, dest); + evpn_route_select_install(bgp, vpn, dest, pi); bgp_dest_unlock_node(dest); /* If this is a new route or some attribute has changed, export the diff --git a/bgpd/bgp_evpn_private.h b/bgpd/bgp_evpn_private.h index 5af99afa34..07bba9b426 100644 --- a/bgpd/bgp_evpn_private.h +++ b/bgpd/bgp_evpn_private.h @@ -716,7 +716,8 @@ extern void delete_evpn_route_entry(struct bgp *bgp, afi_t afi, safi_t safi, struct bgp_path_info **pi); int vni_list_cmp(void *p1, void *p2); extern int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn, - struct bgp_dest *dest); + struct bgp_dest *dest, + struct bgp_path_info *pi); extern struct bgp_dest * bgp_evpn_global_node_get(struct bgp_table *table, afi_t afi, safi_t safi, const struct prefix_evpn *evp, struct prefix_rd *prd, From 6ebb7add1f9191af1a8d5e156e0525b1b0cc9970 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 20 Mar 2024 10:13:00 -0400 Subject: [PATCH 10/11] bgpd: Do not reap, schedule for deletion Do not reap instead let's schedule for deletion and let best_path_selection take care of the deletion as it should. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index 5f3f0f923c..f82dc97aca 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2383,8 +2383,7 @@ static int delete_evpn_route(struct bgp *bgp, struct bgpevpn *vpn, */ delete_evpn_route_entry(bgp, afi, safi, dest, &pi); if (pi) { - dest = bgp_path_info_reap(dest, pi); - assert(dest); + bgp_path_info_delete(dest, pi); evpn_route_select_install(bgp, vpn, dest, pi); } From f3575f61c7c15a4207006043eb359e390751ce3b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 19 Mar 2024 12:26:14 -0400 Subject: [PATCH 11/11] bgpd: Sort the bgp_path_info's Currently bgp_path_info's are stored in reverse order received. Sort them by the best path ordering. This will allow for optimizations in the future on how multipath is done. Signed-off-by: Donald Sharp --- bgpd/bgp_evpn.c | 20 ++- bgpd/bgp_mplsvpn.c | 19 ++- bgpd/bgp_route.c | 367 +++++++++++++++++++++++++++++++++++++-------- bgpd/bgp_route.h | 2 + 4 files changed, 337 insertions(+), 71 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index f82dc97aca..a799cf76f2 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -1440,13 +1440,26 @@ static void evpn_delete_old_local_route(struct bgp *bgp, struct bgpevpn *vpn, int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn, struct bgp_dest *dest, struct bgp_path_info *pi) { - struct bgp_path_info *old_select, *new_select; + struct bgp_path_info *old_select, *new_select, *first; struct bgp_path_info_pair old_and_new; afi_t afi = AFI_L2VPN; safi_t safi = SAFI_EVPN; int ret = 0; + first = bgp_dest_get_bgp_path_info(dest); SET_FLAG(pi->flags, BGP_PATH_UNSORTED); + if (pi != first) { + if (pi->next) + pi->next->prev = pi->prev; + if (pi->prev) + pi->prev->next = pi->next; + + if (first) + first->prev = pi; + pi->next = first; + pi->prev = NULL; + bgp_dest_set_bgp_path_info(dest, pi); + } /* Compute the best path. */ bgp_best_selection(bgp, dest, &bgp->maxpaths[afi][safi], &old_and_new, @@ -6429,9 +6442,10 @@ void bgp_filter_evpn_routes_upon_martian_change( for (dest = bgp_table_top(table); dest; dest = bgp_route_next(dest)) { + struct bgp_path_info *next; - for (pi = bgp_dest_get_bgp_path_info(dest); pi; - pi = pi->next) { + for (pi = bgp_dest_get_bgp_path_info(dest); + (pi != NULL) && (next = pi->next, 1); pi = next) { bool affected = false; const struct prefix *p; diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 06b1f52246..2404ceffb1 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1976,7 +1976,7 @@ void vpn_leak_from_vrf_withdraw_all(struct bgp *to_bgp, struct bgp *from_bgp, struct bgp_table *table; struct bgp_dest *bn; - struct bgp_path_info *bpi; + struct bgp_path_info *bpi, *next; /* This is the per-RD table of prefixes */ table = bgp_dest_get_bgp_table_info(pdest); @@ -1991,7 +1991,8 @@ void vpn_leak_from_vrf_withdraw_all(struct bgp *to_bgp, struct bgp *from_bgp, __func__, bn); } - for (; bpi; bpi = bpi->next) { + for (; (bpi != NULL) && (next = bpi->next, 1); + bpi = next) { if (debug) zlog_debug("%s: type %d, sub_type %d", __func__, bpi->type, @@ -2515,7 +2516,7 @@ void vpn_leak_to_vrf_withdraw(struct bgp_path_info *path_vpn) void vpn_leak_to_vrf_withdraw_all(struct bgp *to_bgp, afi_t afi) { struct bgp_dest *bn; - struct bgp_path_info *bpi; + struct bgp_path_info *bpi, *next; safi_t safi = SAFI_UNICAST; int debug = BGP_DEBUG(vpn, VPN_LEAK_TO_VRF); @@ -2526,9 +2527,8 @@ void vpn_leak_to_vrf_withdraw_all(struct bgp *to_bgp, afi_t afi) */ for (bn = bgp_table_top(to_bgp->rib[afi][safi]); bn; bn = bgp_route_next(bn)) { - - for (bpi = bgp_dest_get_bgp_path_info(bn); bpi; - bpi = bpi->next) { + for (bpi = bgp_dest_get_bgp_path_info(bn); + (bpi != NULL) && (next = bpi->next, 1); bpi = next) { if (bpi->extra && bpi->extra->vrfleak && bpi->extra->vrfleak->bgp_orig != to_bgp && bpi->extra->vrfleak->parent && @@ -2567,8 +2567,11 @@ void vpn_leak_no_retain(struct bgp *to_bgp, struct bgp *vpn_from, afi_t afi) continue; for (bn = bgp_table_top(table); bn; bn = bgp_route_next(bn)) { - for (bpi = bgp_dest_get_bgp_path_info(bn); bpi; - bpi = bpi->next) { + struct bgp_path_info *next; + + for (bpi = bgp_dest_get_bgp_path_info(bn); + (bpi != NULL) && (next = bpi->next, 1); + bpi = next) { if (bpi->extra && bpi->extra->vrfleak && bpi->extra->vrfleak->bgp_orig == to_bgp) continue; diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 661d20bc94..6599a71808 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -482,6 +482,7 @@ void bgp_path_info_add_with_caller(const char *name, struct bgp_dest *dest, top->prev = pi; bgp_dest_set_bgp_path_info(dest, pi); + SET_FLAG(pi->flags, BGP_PATH_UNSORTED); bgp_path_info_lock(pi); bgp_dest_lock_node(dest); peer_lock(pi->peer); /* bgp_path_info peer reference */ @@ -502,9 +503,27 @@ struct bgp_dest *bgp_path_info_reap(struct bgp_dest *dest, bgp_dest_set_bgp_path_info(dest, pi->next); bgp_path_info_mpath_dequeue(pi); - bgp_path_info_unlock(pi); + + pi->next = NULL; + pi->prev = NULL; + hook_call(bgp_snmp_update_stats, dest, pi, false); + bgp_path_info_unlock(pi); + return bgp_dest_unlock_node(dest); +} + +static struct bgp_dest *bgp_path_info_reap_unsorted(struct bgp_dest *dest, + struct bgp_path_info *pi) +{ + bgp_path_info_mpath_dequeue(pi); + + pi->next = NULL; + pi->prev = NULL; + + hook_call(bgp_snmp_update_stats, dest, pi, false); + bgp_path_info_unlock(pi); + return bgp_dest_unlock_node(dest); } @@ -2782,17 +2801,18 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest, struct bgp_path_info_pair *result, afi_t afi, safi_t safi) { - struct bgp_path_info *new_select; - struct bgp_path_info *old_select; + struct bgp_path_info *new_select, *look_thru; + struct bgp_path_info *old_select, *worse, *first; struct bgp_path_info *pi; struct bgp_path_info *pi1; struct bgp_path_info *pi2; - struct bgp_path_info *nextpi = NULL; int paths_eq, do_mpath; - bool debug; + bool debug, any_comparisons; struct list mp_list; char pfx_buf[PREFIX2STR_BUFFER] = {}; char path_buf[PATH_ADDPATH_STR_BUFFER]; + enum bgp_path_selection_reason reason = bgp_path_selection_none; + bool unsorted_items = true; bgp_mp_list_init(&mp_list); do_mpath = @@ -2803,16 +2823,16 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest, if (debug) prefix2str(bgp_dest_get_prefix(dest), pfx_buf, sizeof(pfx_buf)); - dest->reason = bgp_path_selection_none; /* bgp deterministic-med */ new_select = NULL; if (CHECK_FLAG(bgp->flags, BGP_FLAG_DETERMINISTIC_MED)) { - /* Clear BGP_PATH_DMED_SELECTED for all paths */ for (pi1 = bgp_dest_get_bgp_path_info(dest); pi1; - pi1 = pi1->next) + pi1 = pi1->next) { bgp_path_info_unset_flag(dest, pi1, BGP_PATH_DMED_SELECTED); + UNSET_FLAG(pi1->flags, BGP_PATH_DMED_CHECK); + } for (pi1 = bgp_dest_get_bgp_path_info(dest); pi1; pi1 = pi1->next) { @@ -2875,69 +2895,273 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest, } } - /* Check old selected route and new selected route. */ - old_select = NULL; - new_select = NULL; - for (pi = bgp_dest_get_bgp_path_info(dest); - (pi != NULL) && (nextpi = pi->next, 1); pi = nextpi) { - enum bgp_path_selection_reason reason; + /* + * Let's grab the unsorted items from the list + */ + struct bgp_path_info *unsorted_list = NULL; + struct bgp_path_info *unsorted_list_spot = NULL; + struct bgp_path_info *unsorted_holddown = NULL; + + old_select = NULL; + pi = bgp_dest_get_bgp_path_info(dest); + while (pi && CHECK_FLAG(pi->flags, BGP_PATH_UNSORTED)) { + struct bgp_path_info *next = pi->next; - UNSET_FLAG(pi->flags, BGP_PATH_UNSORTED); if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) old_select = pi; - if (BGP_PATH_HOLDDOWN(pi)) { - /* reap REMOVED routes, if needs be + /* + * Pull off pi off the list + */ + if (pi->next) + pi->next->prev = NULL; + + bgp_dest_set_bgp_path_info(dest, pi->next); + pi->next = NULL; + pi->prev = NULL; + + /* + * Place it on the unsorted list + */ + if (unsorted_list_spot) { + unsorted_list_spot->next = pi; + pi->prev = unsorted_list_spot; + pi->next = NULL; + } else { + unsorted_list = pi; + + pi->next = NULL; + pi->prev = NULL; + } + + unsorted_list_spot = pi; + pi = next; + } + + if (!old_select) { + old_select = bgp_dest_get_bgp_path_info(dest); + if (old_select && + !CHECK_FLAG(old_select->flags, BGP_PATH_SELECTED)) + old_select = NULL; + } + + if (!unsorted_list) + unsorted_items = true; + else + unsorted_items = false; + + any_comparisons = false; + worse = NULL; + while (unsorted_list) { + first = unsorted_list; + unsorted_list = unsorted_list->next; + + if (unsorted_list) + unsorted_list->prev = NULL; + first->next = NULL; + first->prev = NULL; + + /* + * It's not likely that the just received unsorted entry + * is in holddown and scheduled for removal but we should + * check + */ + if (BGP_PATH_HOLDDOWN(first)) { + /* + * reap REMOVED routes, if needs be * selected route must stay for a while longer though */ if (debug) - zlog_debug( - "%s: %pBD(%s) pi from %s in holddown", - __func__, dest, bgp->name_pretty, - pi->peer->host); + zlog_debug("%s: %pBD(%s) pi %p from %s in holddown", + __func__, dest, bgp->name_pretty, + first, first->peer->host); - if (CHECK_FLAG(pi->flags, BGP_PATH_REMOVED) && - (pi != old_select)) { - dest = bgp_path_info_reap(dest, pi); + if (old_select != first && + CHECK_FLAG(first->flags, BGP_PATH_REMOVED)) { + dest = bgp_path_info_reap_unsorted(dest, first); assert(dest); - } + } else { + /* + * We are in hold down, so we cannot sort this + * item yet. Let's wait, so hold the unsorted + * to the side + */ + if (unsorted_holddown) { + first->next = unsorted_holddown; + unsorted_holddown->prev = first; + unsorted_holddown = first; + } else + unsorted_holddown = first; + UNSET_FLAG(first->flags, BGP_PATH_UNSORTED); + } continue; } - if (pi->peer && pi->peer != bgp->peer_self - && !CHECK_FLAG(pi->peer->sflags, PEER_STATUS_NSF_WAIT)) - if (!peer_established(pi->peer->connection)) { + bgp_path_info_unset_flag(dest, first, BGP_PATH_DMED_CHECK); + + worse = NULL; + + struct bgp_path_info *look_thru_next; + + for (look_thru = bgp_dest_get_bgp_path_info(dest); look_thru; + look_thru = look_thru_next) { + /* look thru can be reaped save the next pointer */ + look_thru_next = look_thru->next; + + /* + * Now we have the first unsorted and the best selected + * Let's do best path comparison + */ + if (BGP_PATH_HOLDDOWN(look_thru)) { + /* reap REMOVED routes, if needs be + * selected route must stay for a while longer though + */ if (debug) - zlog_debug( - "%s: %pBD(%s) non self peer %s not estab state", - __func__, dest, - bgp->name_pretty, - pi->peer->host); + zlog_debug("%s: %pBD(%s) pi from %s %p in holddown", + __func__, dest, + bgp->name_pretty, + look_thru->peer->host, + look_thru); + + if (CHECK_FLAG(look_thru->flags, + BGP_PATH_REMOVED) && + (look_thru != old_select)) { + dest = bgp_path_info_reap(dest, + look_thru); + assert(dest); + } continue; } - bgp_path_info_unset_flag(dest, pi, BGP_PATH_DMED_CHECK); + if (look_thru->peer && + look_thru->peer != bgp->peer_self && + !CHECK_FLAG(look_thru->peer->sflags, + PEER_STATUS_NSF_WAIT)) + if (!peer_established( + look_thru->peer->connection)) { + if (debug) + zlog_debug("%s: %pBD(%s) non self peer %s not estab state", + __func__, dest, + bgp->name_pretty, + look_thru->peer->host); - if (CHECK_FLAG(bgp->flags, BGP_FLAG_DETERMINISTIC_MED) && - (!CHECK_FLAG(pi->flags, BGP_PATH_DMED_SELECTED))) { - if (debug) - zlog_debug("%s: %pBD(%s) pi %s dmed", __func__, - dest, bgp->name_pretty, - pi->peer->host); - continue; + continue; + } + + bgp_path_info_unset_flag(dest, look_thru, + BGP_PATH_DMED_CHECK); + if (CHECK_FLAG(bgp->flags, BGP_FLAG_DETERMINISTIC_MED) && + (!CHECK_FLAG(look_thru->flags, + BGP_PATH_DMED_SELECTED))) { + bgp_path_info_unset_flag(dest, look_thru, + BGP_PATH_DMED_CHECK); + if (debug) + zlog_debug("%s: %pBD(%s) pi %s dmed", + __func__, dest, + bgp->name_pretty, + look_thru->peer->host); + + worse = look_thru; + continue; + } + + reason = dest->reason; + any_comparisons = true; + if (bgp_path_info_cmp(bgp, first, look_thru, &paths_eq, + mpath_cfg, debug, pfx_buf, afi, + safi, &reason)) { + first->reason = reason; + worse = look_thru; + /* + * We can stop looking + */ + break; + } + + look_thru->reason = reason; } - reason = dest->reason; - if (bgp_path_info_cmp(bgp, pi, new_select, &paths_eq, mpath_cfg, - debug, pfx_buf, afi, safi, - &dest->reason)) { - if (new_select == NULL && - reason != bgp_path_selection_none) - dest->reason = reason; - new_select = pi; + if (!any_comparisons) + first->reason = bgp_path_selection_first; + + /* + * At this point worse if NON-NULL is where the first + * pointer should be before. if worse is NULL then + * first is bestpath too. Let's remove first from the + * list and place it in the right spot + */ + + if (!worse) { + struct bgp_path_info *end = + bgp_dest_get_bgp_path_info(dest); + + for (; end && end->next != NULL; end = end->next) + ; + + if (end) + end->next = first; + else + bgp_dest_set_bgp_path_info(dest, first); + first->prev = end; + first->next = NULL; + + dest->reason = first->reason; + } else { + if (worse->prev) + worse->prev->next = first; + first->next = worse; + if (worse) { + first->prev = worse->prev; + worse->prev = first; + } else + first->prev = NULL; + + if (dest->info == worse) { + bgp_dest_set_bgp_path_info(dest, first); + dest->reason = first->reason; + } } + UNSET_FLAG(first->flags, BGP_PATH_UNSORTED); + } + + if (!unsorted_items) { + new_select = bgp_dest_get_bgp_path_info(dest); + while (new_select && BGP_PATH_HOLDDOWN(new_select)) + new_select = new_select->next; + + if (new_select) { + if (new_select->reason == bgp_path_selection_none) + new_select->reason = bgp_path_selection_first; + else if (new_select == bgp_dest_get_bgp_path_info(dest) && + new_select->next == NULL) + new_select->reason = bgp_path_selection_first; + dest->reason = new_select->reason; + } else + dest->reason = bgp_path_selection_none; + } else + new_select = old_select; + + + /* + * Reinsert all the unsorted_holddown items for future processing + * at the end of the list. + */ + if (unsorted_holddown) { + struct bgp_path_info *top = bgp_dest_get_bgp_path_info(dest); + struct bgp_path_info *prev = NULL; + + while (top != NULL) { + prev = top; + top = top->next; + } + + if (prev) { + prev->next = unsorted_holddown; + unsorted_holddown->prev = prev; + } else + bgp_dest_set_bgp_path_info(dest, unsorted_holddown); } /* Now that we know which path is the bestpath see if any of the other @@ -3511,7 +3735,8 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, bgp_path_info_unset_flag(dest, old_select, BGP_PATH_SELECTED); if (new_select) { if (debug) - zlog_debug("%s: setting SELECTED flag", __func__); + zlog_debug("%s: %pBD setting SELECTED flag", __func__, + dest); bgp_path_info_set_flag(dest, new_select, BGP_PATH_SELECTED); bgp_path_info_unset_flag(dest, new_select, BGP_PATH_ATTR_CHANGED); @@ -3747,9 +3972,25 @@ void bgp_process(struct bgp *bgp, struct bgp_dest *dest, * situation, even if the node is already * scheduled. */ - if (pi) + if (pi) { + struct bgp_path_info *first = bgp_dest_get_bgp_path_info(dest); + SET_FLAG(pi->flags, BGP_PATH_UNSORTED); + if (pi != first) { + if (pi->next) + pi->next->prev = pi->prev; + if (pi->prev) + pi->prev->next = pi->next; + + if (first) + first->prev = pi; + pi->next = first; + pi->prev = NULL; + bgp_dest_set_bgp_path_info(dest, pi); + } + } + /* already scheduled for processing? */ if (CHECK_FLAG(dest->flags, BGP_NODE_PROCESS_SCHEDULED)) return; @@ -5637,7 +5878,7 @@ static wq_item_status bgp_clear_route_node(struct work_queue *wq, void *data) struct bgp_clear_node_queue *cnq = data; struct bgp_dest *dest = cnq->dest; struct peer *peer = wq->spec.data; - struct bgp_path_info *pi; + struct bgp_path_info *pi, *next; struct bgp *bgp; afi_t afi = bgp_dest_table(dest)->afi; safi_t safi = bgp_dest_table(dest)->safi; @@ -5648,7 +5889,8 @@ static wq_item_status bgp_clear_route_node(struct work_queue *wq, void *data) /* It is possible that we have multiple paths for a prefix from a peer * if that peer is using AddPath. */ - for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { + for (pi = bgp_dest_get_bgp_path_info(dest); + (pi != NULL) && (next = pi->next, 1); pi = next) { if (pi->peer != peer) continue; @@ -5910,7 +6152,7 @@ void bgp_clear_adj_in(struct peer *peer, afi_t afi, safi_t safi) void bgp_clear_stale_route(struct peer *peer, afi_t afi, safi_t safi) { struct bgp_dest *dest; - struct bgp_path_info *pi; + struct bgp_path_info *pi, *next; struct bgp_table *table; if (safi == SAFI_MPLS_VPN || safi == SAFI_ENCAP || safi == SAFI_EVPN) { @@ -5925,8 +6167,9 @@ void bgp_clear_stale_route(struct peer *peer, afi_t afi, safi_t safi) for (rm = bgp_table_top(table); rm; rm = bgp_route_next(rm)) - for (pi = bgp_dest_get_bgp_path_info(rm); pi; - pi = pi->next) { + for (pi = bgp_dest_get_bgp_path_info(rm); + (pi != NULL) && (next = pi->next, 1); + pi = next) { if (pi->peer != peer) continue; if (CHECK_FLAG( @@ -5959,8 +6202,8 @@ void bgp_clear_stale_route(struct peer *peer, afi_t afi, safi_t safi) } else { for (dest = bgp_table_top(peer->bgp->rib[afi][safi]); dest; dest = bgp_route_next(dest)) - for (pi = bgp_dest_get_bgp_path_info(dest); pi; - pi = pi->next) { + for (pi = bgp_dest_get_bgp_path_info(dest); + (pi != NULL) && (next = pi->next, 1); pi = next) { if (pi->peer != peer) continue; if (CHECK_FLAG(peer->af_sflags[afi][safi], @@ -6670,6 +6913,7 @@ void bgp_static_withdraw(struct bgp *bgp, const struct prefix *p, afi_t afi, /* Withdraw static BGP route from routing table. */ if (pi) { + SET_FLAG(pi->flags, BGP_PATH_UNSORTED); #ifdef ENABLE_BGP_VNC if (safi == SAFI_MPLS_VPN || safi == SAFI_ENCAP) rfapiProcessWithdraw(pi->peer, NULL, p, prd, pi->attr, @@ -7452,8 +7696,10 @@ static void bgp_aggregate_install( /* * Mark the old as unusable */ - if (pi) + if (pi) { bgp_path_info_delete(dest, pi); + bgp_process(bgp, dest, pi, afi, safi); + } attr = bgp_attr_aggregate_intern( bgp, origin, aspath, community, ecommunity, lcommunity, @@ -7900,7 +8146,8 @@ void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, bgp_process(bgp, dest, pi, afi, safi); } - aggregate->count--; + if (aggregate->count > 0) + aggregate->count--; if (pi->attr->origin == BGP_ORIGIN_INCOMPLETE) aggregate->incomplete_origin_count--; diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index e7dac0ff29..d51d20784f 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -346,6 +346,8 @@ struct bgp_path_info { unsigned short instance; + enum bgp_path_selection_reason reason; + /* Addpath identifiers */ uint32_t addpath_rx_id; struct bgp_addpath_info_data tx_addpath;