From def2c27e49125ad523cd36ed16dd787946640dba Mon Sep 17 00:00:00 2001 From: Mitchell Skiba Date: Thu, 9 Jan 2020 11:46:13 -0800 Subject: [PATCH] bgpd: add addpath ID to adj_out tree sort When withdrawing addpaths, adj_lookup was called to find the path that needed to be withdrawn. It would lookup in the RB tree based on subgroup pointer alone, often find the path with the wrong addpath ID, and return null. Only the path highest in the tree sent to the subgroup could be found, thus withdrawn. Adding the addpath ID to the sort criteria for the RB tree allows us to simplify the logic for adj_lookup, and address this problem. We are able to remove the logic around non-addpath subgroups because the addpath ID is consistently 0 for non-addpath adj_outs, so special logic to skip matching the addpath ID isn't required. (As a side note, addpath will also never use ID 0, so there won't be any ambiguity when looking at the structure content.) Signed-off-by: Mitchell Skiba --- bgpd/bgp_updgrp_adv.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index b64c51f341..dc20234fd7 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -64,6 +64,12 @@ static int bgp_adj_out_compare(const struct bgp_adj_out *o1, if (o1->subgroup > o2->subgroup) return 1; + if (o1->addpath_tx_id < o2->addpath_tx_id) + return -1; + + if (o1->addpath_tx_id > o2->addpath_tx_id) + return 1; + return 0; } RB_GENERATE(bgp_adj_out_rb, bgp_adj_out, adj_entry, bgp_adj_out_compare); @@ -72,32 +78,17 @@ static inline struct bgp_adj_out *adj_lookup(struct bgp_node *rn, struct update_subgroup *subgrp, uint32_t addpath_tx_id) { - struct bgp_adj_out *adj, lookup; - struct peer *peer; - afi_t afi; - safi_t safi; - int addpath_capable; + struct bgp_adj_out lookup; if (!rn || !subgrp) return NULL; - peer = SUBGRP_PEER(subgrp); - afi = SUBGRP_AFI(subgrp); - safi = SUBGRP_SAFI(subgrp); - addpath_capable = bgp_addpath_encode_tx(peer, afi, safi); - /* update-groups that do not support addpath will pass 0 for - * addpath_tx_id so do not both matching against it */ + * addpath_tx_id. */ lookup.subgroup = subgrp; - adj = RB_FIND(bgp_adj_out_rb, &rn->adj_out, &lookup); - if (adj) { - if (addpath_capable) { - if (adj->addpath_tx_id == addpath_tx_id) - return adj; - } else - return adj; - } - return NULL; + lookup.addpath_tx_id = addpath_tx_id; + + return RB_FIND(bgp_adj_out_rb, &rn->adj_out, &lookup); } static void adj_free(struct bgp_adj_out *adj) @@ -402,13 +393,14 @@ struct bgp_adj_out *bgp_adj_out_alloc(struct update_subgroup *subgrp, adj = XCALLOC(MTYPE_BGP_ADJ_OUT, sizeof(struct bgp_adj_out)); adj->subgroup = subgrp; + adj->addpath_tx_id = addpath_tx_id; + if (rn) { RB_INSERT(bgp_adj_out_rb, &rn->adj_out, adj); bgp_lock_node(rn); adj->rn = rn; } - adj->addpath_tx_id = addpath_tx_id; TAILQ_INSERT_TAIL(&(subgrp->adjq), adj, subgrp_adj_train); SUBGRP_INCR_STAT(subgrp, adj_count); return adj;