From 318cac96ef7e16a17fd1ab97b67ede5c07651c52 Mon Sep 17 00:00:00 2001 From: Daniel Walton Date: Tue, 22 Aug 2017 18:14:50 +0000 Subject: [PATCH 1/3] bgpd: Memory wasting in zebra by non used MPLS FECs Signed-off-by: Daniel Walton --- bgpd/bgp_route.c | 7 +++- bgpd/bgp_vty.c | 23 +----------- bgpd/bgpd.c | 95 ++++++++++++++++++++++++++++++++++++++---------- bgpd/bgpd.h | 4 ++ 4 files changed, 85 insertions(+), 44 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index bb204b01f1..0c60231ded 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2045,7 +2045,7 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_node *rn, * to do this upon changes to best path except of the label index * changes. */ - if (safi == SAFI_UNICAST) { + if (bgp->allocate_mpls_labels[afi][safi]) { if (new_select) { if (!old_select || bgp_label_index_differs(new_select, old_select) @@ -2066,8 +2066,11 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_node *rn, } else bgp_register_for_label(rn, new_select); } - } else if (CHECK_FLAG(rn->flags, BGP_NODE_REGISTERED_FOR_LABEL)) + } else if (CHECK_FLAG(rn->flags, BGP_NODE_REGISTERED_FOR_LABEL)) { bgp_unregister_for_label(rn); + } + } else if (CHECK_FLAG(rn->flags, BGP_NODE_REGISTERED_FOR_LABEL)) { + bgp_unregister_for_label(rn); } /* If best route remains the same and this is not due to user-initiated diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 2e18a6d44f..f040ed9a08 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -7018,26 +7018,6 @@ static int bgp_show_summary(struct vty *vty, struct bgp *bgp, int afi, int safi, return CMD_SUCCESS; } -/* - * Return if we have a peer configured to use this afi/safi - */ -static int bgp_show_summary_afi_safi_peer_exists(struct bgp *bgp, int afi, - int safi) -{ - struct listnode *node; - struct peer *peer; - - for (ALL_LIST_ELEMENTS_RO(bgp->peer, node, peer)) { - if (!CHECK_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE)) - continue; - - if (peer->afc[afi][safi]) - return 1; - } - - return 0; -} - static void bgp_show_summary_afi_safi(struct vty *vty, struct bgp *bgp, int afi, int safi, u_char use_json, json_object *json) @@ -7056,8 +7036,7 @@ static void bgp_show_summary_afi_safi(struct vty *vty, struct bgp *bgp, int afi, if (safi_wildcard) safi = 1; /* SAFI_UNICAST */ while (safi < SAFI_MAX) { - if (bgp_show_summary_afi_safi_peer_exists(bgp, afi, - safi)) { + if (bgp_afi_safi_peer_exists(bgp, afi, safi)) { json_output = true; if (is_wildcard) { /* diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 9d7c38c871..ba2e82a42e 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1391,6 +1391,32 @@ void bgp_peer_conf_if_to_su_update(struct peer *peer) hash_get(peer->bgp->peerhash, peer, hash_alloc_intern); } +static void bgp_recalculate_afi_safi_bestpaths(struct bgp *bgp, afi_t afi, + safi_t safi) +{ + struct bgp_node *rn, *nrn; + + for (rn = bgp_table_top(bgp->rib[afi][safi]); rn; + rn = bgp_route_next(rn)) { + if (rn->info != NULL) { + /* Special handling for 2-level routing + * tables. */ + if (safi == SAFI_MPLS_VPN + || safi == SAFI_ENCAP + || safi == SAFI_EVPN) { + for (nrn = bgp_table_top(( + struct bgp_table + *)(rn->info)); + nrn; + nrn = bgp_route_next(nrn)) + bgp_process(bgp, nrn, + afi, safi); + } else + bgp_process(bgp, rn, afi, safi); + } + } +} + /* Force a bestpath recalculation for all prefixes. This is used * when 'bgp bestpath' commands are entered. */ @@ -1398,29 +1424,10 @@ void bgp_recalculate_all_bestpaths(struct bgp *bgp) { afi_t afi; safi_t safi; - struct bgp_node *rn, *nrn; for (afi = AFI_IP; afi < AFI_MAX; afi++) { for (safi = SAFI_UNICAST; safi < SAFI_MAX; safi++) { - for (rn = bgp_table_top(bgp->rib[afi][safi]); rn; - rn = bgp_route_next(rn)) { - if (rn->info != NULL) { - /* Special handling for 2-level routing - * tables. */ - if (safi == SAFI_MPLS_VPN - || safi == SAFI_ENCAP - || safi == SAFI_EVPN) { - for (nrn = bgp_table_top(( - struct bgp_table - *)(rn->info)); - nrn; - nrn = bgp_route_next(nrn)) - bgp_process(bgp, nrn, - afi, safi); - } else - bgp_process(bgp, rn, afi, safi); - } - } + bgp_recalculate_afi_safi_bestpaths(bgp, afi, safi); } } } @@ -1500,6 +1507,25 @@ struct peer *peer_create_accept(struct bgp *bgp) return peer; } +/* + * Return true if we have a peer configured to use this afi/safi + */ +int bgp_afi_safi_peer_exists(struct bgp *bgp, afi_t afi, safi_t safi) +{ + struct listnode *node; + struct peer *peer; + + for (ALL_LIST_ELEMENTS_RO(bgp->peer, node, peer)) { + if (!CHECK_FLAG(peer->flags, PEER_FLAG_CONFIG_NODE)) + continue; + + if (peer->afc[afi][safi]) + return 1; + } + + return 0; +} + /* Change peer's AS number. */ void peer_as_change(struct peer *peer, as_t as, int as_specified) { @@ -1714,11 +1740,14 @@ int peer_activate(struct peer *peer, afi_t afi, safi_t safi) struct peer_group *group; struct listnode *node, *nnode; struct peer *tmp_peer; + struct bgp *bgp; /* Nothing to do if we've already activated this peer */ if (peer->afc[afi][safi]) return ret; + bgp = peer->bgp; + /* This is a peer-group so activate all of the members of the * peer-group as well */ if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) { @@ -1741,6 +1770,17 @@ int peer_activate(struct peer *peer, afi_t afi, safi_t safi) ret |= non_peergroup_activate_af(peer, afi, safi); } + /* If this is the first peer to be activated for this afi/labeled-unicast + * recalc bestpaths to trigger label allocation */ + if (safi == SAFI_LABELED_UNICAST && !bgp->allocate_mpls_labels[afi][SAFI_UNICAST]) { + + if (BGP_DEBUG(zebra, ZEBRA)) + zlog_info("peer(s) are now active for labeled-unicast, allocate MPLS labels"); + + bgp->allocate_mpls_labels[afi][SAFI_UNICAST] = 1; + bgp_recalculate_afi_safi_bestpaths(bgp, afi, SAFI_UNICAST); + } + return ret; } @@ -1798,6 +1838,7 @@ int peer_deactivate(struct peer *peer, afi_t afi, safi_t safi) struct peer_group *group; struct peer *tmp_peer; struct listnode *node, *nnode; + struct bgp *bgp; /* Nothing to do if we've already de-activated this peer */ if (!peer->afc[afi][safi]) @@ -1821,6 +1862,20 @@ int peer_deactivate(struct peer *peer, afi_t afi, safi_t safi) ret |= non_peergroup_deactivate_af(peer, afi, safi); } + bgp = peer->bgp; + + /* If this is the last peer to be deactivated for this afi/labeled-unicast + * recalc bestpaths to trigger label deallocation */ + if (safi == SAFI_LABELED_UNICAST && + bgp->allocate_mpls_labels[afi][SAFI_UNICAST] && + !bgp_afi_safi_peer_exists(bgp, afi, safi)) { + + if (BGP_DEBUG(zebra, ZEBRA)) + zlog_info("peer(s) are no longer active for labeled-unicast, deallocate MPLS labels"); + + bgp->allocate_mpls_labels[afi][SAFI_UNICAST] = 0; + bgp_recalculate_afi_safi_bestpaths(bgp, afi, SAFI_UNICAST); + } return ret; } diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index f6e7b2277f..5ede9ba13d 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -333,6 +333,9 @@ struct bgp { /* BGP redistribute configuration. */ struct list *redist[AFI_MAX][ZEBRA_ROUTE_MAX]; + /* Allocate MPLS labels */ + u_char allocate_mpls_labels[AFI_MAX][SAFI_MAX]; + /* timer to re-evaluate neighbor default-originate route-maps */ struct thread *t_rmap_def_originate_eval; #define RMAP_DEFAULT_ORIGINATE_EVAL_TIMER 5 @@ -1280,6 +1283,7 @@ extern int bgp_listen_limit_unset(struct bgp *); extern int bgp_update_delay_active(struct bgp *); extern int bgp_update_delay_configured(struct bgp *); +extern int bgp_afi_safi_peer_exists(struct bgp *bgp, afi_t afi, safi_t safi); extern void peer_as_change(struct peer *, as_t, int); extern int peer_remote_as(struct bgp *, union sockunion *, const char *, as_t *, int, afi_t, safi_t); From 7b2ff250318a27a03937b1edae68ffbf06714eab Mon Sep 17 00:00:00 2001 From: Daniel Walton Date: Tue, 22 Aug 2017 19:11:31 +0000 Subject: [PATCH 2/3] bgpd: remove json option from 'sh ip bgp' commands that do not support it Signed-off-by: Daniel Walton --- bgpd/bgp_route.c | 114 +++++++++++++++++++++++++++++++---------------- 1 file changed, 75 insertions(+), 39 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 0c60231ded..d54dec269c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -8838,32 +8838,28 @@ DEFUN (show_ip_bgp_large_community, static int bgp_table_stats(struct vty *vty, struct bgp *bgp, afi_t afi, safi_t safi); -/* BGP route print out function. */ + +/* BGP route print out function without JSON */ DEFUN (show_ip_bgp, show_ip_bgp_cmd, "show [ip] bgp [ VIEWVRFNAME] ["BGP_AFI_CMD_STR" ["BGP_SAFI_WITH_LABEL_CMD_STR"]]\ - [<\ - cidr-only\ - |dampening \ - |route-map WORD\ - |prefix-list WORD\ - |filter-list WORD\ - |statistics\ - |community [ [exact-match]]\ - |community-list <(1-500)|WORD> [exact-match]\ - |A.B.C.D/M longer-prefixes\ - |X:X::X:X/M longer-prefixes>\ - ] [json]", + \ + |route-map WORD\ + |prefix-list WORD\ + |filter-list WORD\ + |statistics\ + |community [exact-match]\ + |community-list <(1-500)|WORD> [exact-match]\ + |A.B.C.D/M longer-prefixes\ + |X:X::X:X/M longer-prefixes\ + >", SHOW_STR IP_STR BGP_STR BGP_INSTANCE_HELP_STR BGP_AFI_HELP_STR BGP_SAFI_WITH_LABEL_HELP_STR - "Display only routes with non-natural netmasks\n" "Display detailed information about dampening\n" - "Display flap statistics of routes\n" - "Display paths suppressed due to dampening\n" "Display detail of configured dampening parameters\n" "Display routes matching the route-map\n" "A route-map to match on\n" @@ -8885,13 +8881,11 @@ DEFUN (show_ip_bgp, "IPv4 prefix\n" "Display route and more specific routes\n" "IPv6 prefix\n" - "Display route and more specific routes\n" - JSON_STR) + "Display route and more specific routes\n") { afi_t afi = AFI_IP6; safi_t safi = SAFI_UNICAST; int exact_match = 0; - enum bgp_show_type sh_type = bgp_show_type_normal; struct bgp *bgp = NULL; int idx = 0; @@ -8900,23 +8894,8 @@ DEFUN (show_ip_bgp, if (!idx) return CMD_WARNING; - int uj = use_json(argc, argv); - if (uj) - argc--; - - if (argv_find(argv, argc, "cidr-only", &idx)) - return bgp_show(vty, bgp, afi, safi, bgp_show_type_cidr_only, - NULL, uj); - if (argv_find(argv, argc, "dampening", &idx)) { - if (argv_find(argv, argc, "dampened-paths", &idx)) - return bgp_show(vty, bgp, afi, safi, - bgp_show_type_dampend_paths, NULL, uj); - else if (argv_find(argv, argc, "flap-statistics", &idx)) - return bgp_show(vty, bgp, afi, safi, - bgp_show_type_flap_statistics, NULL, - uj); - else if (argv_find(argv, argc, "parameters", &idx)) + if (argv_find(argv, argc, "parameters", &idx)) return bgp_show_dampening_parameters(vty, afi, safi); } @@ -8945,10 +8924,6 @@ DEFUN (show_ip_bgp, return bgp_show_community(vty, bgp, argc, argv, exact_match, afi, safi); } - /* show all communities */ - else - return bgp_show(vty, bgp, afi, safi, - bgp_show_type_community_all, NULL, uj); } if (argv_find(argv, argc, "community-list", &idx)) { @@ -8965,6 +8940,66 @@ DEFUN (show_ip_bgp, safi, bgp_show_type_prefix_longer); + return CMD_WARNING; +} + +/* BGP route print out function with JSON */ +DEFUN (show_ip_bgp_json, + show_ip_bgp_json_cmd, + "show [ip] bgp [ VIEWVRFNAME] ["BGP_AFI_CMD_STR" ["BGP_SAFI_WITH_LABEL_CMD_STR"]]\ + [<\ + cidr-only\ + |dampening \ + |community \ + >] [json]", + SHOW_STR + IP_STR + BGP_STR + BGP_INSTANCE_HELP_STR + BGP_AFI_HELP_STR + BGP_SAFI_WITH_LABEL_HELP_STR + "Display only routes with non-natural netmasks\n" + "Display detailed information about dampening\n" + "Display flap statistics of routes\n" + "Display paths suppressed due to dampening\n" + "Display routes matching the communities\n" + JSON_STR) +{ + afi_t afi = AFI_IP6; + safi_t safi = SAFI_UNICAST; + enum bgp_show_type sh_type = bgp_show_type_normal; + struct bgp *bgp = NULL; + int idx = 0; + + bgp_vty_find_and_parse_afi_safi_bgp(vty, argv, argc, &idx, &afi, &safi, + &bgp); + if (!idx) + return CMD_WARNING; + + int uj = use_json(argc, argv); + if (uj) + argc--; + + if (argv_find(argv, argc, "cidr-only", &idx)) + return bgp_show(vty, bgp, afi, safi, bgp_show_type_cidr_only, + NULL, uj); + + if (argv_find(argv, argc, "dampening", &idx)) { + if (argv_find(argv, argc, "dampened-paths", &idx)) + return bgp_show(vty, bgp, afi, safi, + bgp_show_type_dampend_paths, NULL, uj); + else if (argv_find(argv, argc, "flap-statistics", &idx)) + return bgp_show(vty, bgp, afi, safi, + bgp_show_type_flap_statistics, NULL, + uj); + } + + if (argv_find(argv, argc, "community", &idx)) { + /* show all communities */ + return bgp_show(vty, bgp, afi, safi, + bgp_show_type_community_all, NULL, uj); + } + if (safi == SAFI_MPLS_VPN) return bgp_show_mpls_vpn(vty, afi, NULL, bgp_show_type_normal, NULL, 0, uj); @@ -11283,6 +11318,7 @@ void bgp_route_init(void) /* IPv4 labeled-unicast configuration. */ install_element(VIEW_NODE, &show_ip_bgp_instance_all_cmd); install_element(VIEW_NODE, &show_ip_bgp_cmd); + install_element(VIEW_NODE, &show_ip_bgp_json_cmd); install_element(VIEW_NODE, &show_ip_bgp_route_cmd); install_element(VIEW_NODE, &show_ip_bgp_regexp_cmd); From e889891d8b543a93521e4d15a51770daf482b979 Mon Sep 17 00:00:00 2001 From: Daniel Walton Date: Tue, 22 Aug 2017 19:33:22 +0000 Subject: [PATCH 3/3] bgpd: 'show ip bgp regex' needs to be VRF aware Signed-off-by: Daniel Walton --- bgpd/bgp_route.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index d54dec269c..cc9e902eb1 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -8020,7 +8020,8 @@ static int bgp_show_community_list(struct vty *vty, struct bgp *bgp, static int bgp_show_prefix_longer(struct vty *vty, struct bgp *bgp, const char *prefix, afi_t afi, safi_t safi, enum bgp_show_type type); -static int bgp_show_regexp(struct vty *vty, const char *regstr, afi_t afi, +static int bgp_show_regexp(struct vty *vty, struct bgp *bgp, + const char *regstr, afi_t afi, safi_t safi, enum bgp_show_type type); static int bgp_show_community(struct vty *vty, struct bgp *bgp, int argc, struct cmd_token **argv, int exact, afi_t afi, @@ -9109,7 +9110,7 @@ DEFUN (show_ip_bgp_regexp, idx++; char *regstr = argv_concat(argv, argc, idx); - int rc = bgp_show_regexp(vty, (const char *)regstr, afi, safi, + int rc = bgp_show_regexp(vty, bgp, (const char *)regstr, afi, safi, bgp_show_type_regexp); XFREE(MTYPE_TMP, regstr); return rc; @@ -9144,7 +9145,8 @@ DEFUN (show_ip_bgp_instance_all, return CMD_SUCCESS; } -static int bgp_show_regexp(struct vty *vty, const char *regstr, afi_t afi, +static int bgp_show_regexp(struct vty *vty, struct bgp *bgp, + const char *regstr, afi_t afi, safi_t safi, enum bgp_show_type type) { regex_t *regex; @@ -9156,7 +9158,7 @@ static int bgp_show_regexp(struct vty *vty, const char *regstr, afi_t afi, return CMD_WARNING; } - rc = bgp_show(vty, NULL, afi, safi, type, regex, 0); + rc = bgp_show(vty, bgp, afi, safi, type, regex, 0); bgp_regex_free(regex); return rc; }