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 <mskiba@amazon.com>

diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c
index b64c51f34140aef2caad45f8ec96aa954cb4dcbf..dc20234fd7e4a49c7827ede187a067d396a5d475 100644 (file)
--- 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;