lib, bgpd: add "unique mode" for route tables, supporting direct lookup only - #20589
Conversation
|
What is the performance gain here? As I understand, this should be relevant to all VPN tables, not only EVPN? Or did I miss something? |
|
yes, you're quite right - there should be a benefit for any of the tables that don't use IP prefixes. I haven't got memory numbers yet - that's the real benefit, I think, saving the memory that the interior/internal route-nodes use.
|
384a5fb to
866409b
Compare
|
pushed an update to fix the checkpatch warning |
866409b to
b7ff14b
Compare
|
and another fixup |
|
I maaaaaaay (no promises) actually have some numbers for the RB-tree part due to having tried this before; I was tempted to replace the LPM lookup in general with RB-tree search-next operations (if you do the sort order right, you can do that…) but it kinda turned out a mixed bag for that use case. I didn't think about a flag to switch a table into "unique mode" there… |
b7ff14b to
baf4574
Compare
|
I'm personally convinced that this is ready to go. |
|
Clearing the 'draft' status |
Greptile OverviewGreptile SummaryThis PR introduces a new The main integration risk is that several existing code paths in bgpd/lib still assume trie semantics ( Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant BGP as bgpd/bgp_table.c
participant RT as lib/table.c
participant HASH as rn_hash_node
participant TREE as rn_tree
BGP->>RT: route_table_init_with_delegate()
RT->>HASH: rn_hash_node_init()
RT->>TREE: rn_tree_init()
opt SAFI_EVPN
BGP->>RT: route_table_set_unique_mode()
note over RT: unique_mode=true
end
BGP->>RT: route_node_get(prefix)
alt unique_mode
RT->>HASH: rn_hash_node_find(prefix)
alt not found
RT->>HASH: rn_hash_node_add(new_node)
RT->>TREE: rn_tree_add(new_node)
end
else prefix-trie mode
RT->>RT: walk/extend trie
RT->>HASH: rn_hash_node_add(new_node)
end
BGP->>RT: route_top()/route_next()
alt unique_mode
RT->>TREE: rn_tree_first()/rn_tree_next()
else trie mode
RT->>RT: traverse via link/parent
end
BGP->>RT: route_table_finish()
RT->>RT: route_table_free()
note over RT: free currently uses rt->top traversal
RT->>HASH: rn_hash_node_fini()
RT->>TREE: rn_tree_fini()
|
3 similar comments
Greptile OverviewGreptile SummaryThis PR introduces a new The main integration risk is that several existing code paths in bgpd/lib still assume trie semantics ( Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant BGP as bgpd/bgp_table.c
participant RT as lib/table.c
participant HASH as rn_hash_node
participant TREE as rn_tree
BGP->>RT: route_table_init_with_delegate()
RT->>HASH: rn_hash_node_init()
RT->>TREE: rn_tree_init()
opt SAFI_EVPN
BGP->>RT: route_table_set_unique_mode()
note over RT: unique_mode=true
end
BGP->>RT: route_node_get(prefix)
alt unique_mode
RT->>HASH: rn_hash_node_find(prefix)
alt not found
RT->>HASH: rn_hash_node_add(new_node)
RT->>TREE: rn_tree_add(new_node)
end
else prefix-trie mode
RT->>RT: walk/extend trie
RT->>HASH: rn_hash_node_add(new_node)
end
BGP->>RT: route_top()/route_next()
alt unique_mode
RT->>TREE: rn_tree_first()/rn_tree_next()
else trie mode
RT->>RT: traverse via link/parent
end
BGP->>RT: route_table_finish()
RT->>RT: route_table_free()
note over RT: free currently uses rt->top traversal
RT->>HASH: rn_hash_node_fini()
RT->>TREE: rn_tree_fini()
|
Greptile OverviewGreptile SummaryThis PR introduces a new The main integration risk is that several existing code paths in bgpd/lib still assume trie semantics ( Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant BGP as bgpd/bgp_table.c
participant RT as lib/table.c
participant HASH as rn_hash_node
participant TREE as rn_tree
BGP->>RT: route_table_init_with_delegate()
RT->>HASH: rn_hash_node_init()
RT->>TREE: rn_tree_init()
opt SAFI_EVPN
BGP->>RT: route_table_set_unique_mode()
note over RT: unique_mode=true
end
BGP->>RT: route_node_get(prefix)
alt unique_mode
RT->>HASH: rn_hash_node_find(prefix)
alt not found
RT->>HASH: rn_hash_node_add(new_node)
RT->>TREE: rn_tree_add(new_node)
end
else prefix-trie mode
RT->>RT: walk/extend trie
RT->>HASH: rn_hash_node_add(new_node)
end
BGP->>RT: route_top()/route_next()
alt unique_mode
RT->>TREE: rn_tree_first()/rn_tree_next()
else trie mode
RT->>RT: traverse via link/parent
end
BGP->>RT: route_table_finish()
RT->>RT: route_table_free()
note over RT: free currently uses rt->top traversal
RT->>HASH: rn_hash_node_fini()
RT->>TREE: rn_tree_fini()
|
Greptile OverviewGreptile SummaryThis PR introduces a new The main integration risk is that several existing code paths in bgpd/lib still assume trie semantics ( Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant BGP as bgpd/bgp_table.c
participant RT as lib/table.c
participant HASH as rn_hash_node
participant TREE as rn_tree
BGP->>RT: route_table_init_with_delegate()
RT->>HASH: rn_hash_node_init()
RT->>TREE: rn_tree_init()
opt SAFI_EVPN
BGP->>RT: route_table_set_unique_mode()
note over RT: unique_mode=true
end
BGP->>RT: route_node_get(prefix)
alt unique_mode
RT->>HASH: rn_hash_node_find(prefix)
alt not found
RT->>HASH: rn_hash_node_add(new_node)
RT->>TREE: rn_tree_add(new_node)
end
else prefix-trie mode
RT->>RT: walk/extend trie
RT->>HASH: rn_hash_node_add(new_node)
end
BGP->>RT: route_top()/route_next()
alt unique_mode
RT->>TREE: rn_tree_first()/rn_tree_next()
else trie mode
RT->>RT: traverse via link/parent
end
BGP->>RT: route_table_finish()
RT->>RT: route_table_free()
note over RT: free currently uses rt->top traversal
RT->>HASH: rn_hash_node_fini()
RT->>TREE: rn_tree_fini()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/table.c
Line: 108:112
Comment:
**Unique-mode tables never freed**
`route_table_free()` (lib/table.c:108+) only walks `rt->top`/`parent`/`l_left`/`l_right` to bulk-delete nodes. In `unique_mode`, `rt->top` is never populated and nodes are instead stored in `rt->tree`, so the loop never runs and the `assert(rt->count == 0)` will fire if the table is finished with live nodes (and even if `count` were forced to 0, nodes would leak via the hash unless explicitly walked). This needs a unique-mode free path that iterates the rbtree (or otherwise deletes all hash/tree entries) before finalizing.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: bgpd/bgp_table.c
Line: 214:218
Comment:
**EVPN subtree lookup broken**
`bgp_table_subtree_lookup()` (bgpd/bgp_table.c:214+) directly traverses `table->route_table->top` and `node->link[...]` assuming a prefix-trie. For SAFI_EVPN tables you now enable `unique_mode` in `bgp_table_init()` (bgpd/bgp_table.c:160-162), where `top/link/parent` aren’t maintained, so this function will always return NULL (because `top` stays NULL) or behave incorrectly if it ever did get called on an EVPN table. Callers need to be prevented from using this on EVPN/unique-mode tables, or the implementation needs a unique-mode equivalent.
How can I resolve this? If you propose a fix, please make it concise. |
|
greptile has a valid question, I think, so let's not merge this just yet |
baf4574 to
f80e155
Compare
|
resolved comments and rebased |
|
CI:rerun |
|
I don't think the lint errors are worth worrying about here (?) |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Add a mode for route tables that do not use LPM prefix matching - such as EVPN tables. Signed-off-by: Mark Stapp <mjs@cisco.com>
Use the new unique, non-prefix mode for evpn route tables. Signed-off-by: Mark Stapp <mjs@cisco.com>
f80e155 to
72c388e
Compare
|
rebased and resolved conflicts |
no, they're about super-wide lines... optional |
5 participants
The existing route-table library was designed to support ip prefixes, where there's a hierarchy of prefixes with parents and children. Some of our route tables don't have this property - the items in the table don't have a hierarchy in the same way as ip prefixes. In those cases, the internal hierarchy of route-node objects just occupies memory without providing value.
This PR adds a "unique" mode that doesn't create any internal hierarchy - it uses just direct lookups.
Here's a before and after comparison, with 50000 static routes redistributed into an evpn l3vni in the
bgp_evpn_rt5topotest. I added a temporary "show bgp tables" command that shows the count of route-nodes in some of the interesting bgp route-tables.With the current table scheme, both the EVPN and unicast tables consume 100K nodes to hold the 50K entries, with the corresponding memory footprint:
The after version with bgp EVPN tables using the new unique-mode in this PR; the EVPN table only requires 50K nodes for the 50K RT5s.