bgpd: Optimize BGP path lookup using typesafe hash for efficient lookup #20331
Conversation
Test Results:Overall scale: 32k routes * 128 way ECMP: ~4 million pathsWithout Fix:Event CPU: With Fix:Event CPU: ~60% improvement |
krishna-samy
force-pushed
the
krishna/bgp-path-lookup
branch
from
0093961 to
61983d3
Compare
Flamegraph showing the cpu cycles in bgp_update() during NLRI processingWithout Fix: With Fix: bgp_update is improved a lot |
krishna-samy
force-pushed
the
krishna/bgp-path-lookup
branch
from
61983d3 to
b2d8f2c
Compare
bgpd/bgp_route.c
Outdated
| * Initialize typesafe hash on first path add. | ||
| * XCALLOC zeroes dest, so count is 0 initially. | ||
| */ | ||
| if (bgp_pi_hash_count(&dest->pi_hash) == 0) |
There was a problem hiding this comment.
This approach seems fragile and prone to problems in the future to me. When we create the dest why not init there?
There was a problem hiding this comment.
What do you think of having the hash stuck off the table w/ the prefix as part of the key instead? Wouldn't this reduce the memory footprint?
There was a problem hiding this comment.
What do you think of having the hash stuck off the table w/ the prefix as part of the key instead? Wouldn't this reduce the memory footprint?
Do you mean having a global hash per AFI/SAFI instead of per dest?
In that case, yes we would save the per dest hash_head - which is calculated below.
let's assume high route scenario - 1.2 million paths and 4 way ecmp
--> 1.2M × 16bytes(hash_head) = 19,200,000 bytes = ~18 MB
But in case of low route(16k/32k) and high ECMP(256) scenario, the memory footprint would be pretty much same based on the # of buckets allocation.
So, we consume around ~130MB with existing per dest hash. We save ~18MB if we move to global hash.
I have couple of questions in that case.
- Larger buckets needed. Must accommodate ALL paths in table. 5M entries in one hash would be ok?
- Must remove all paths for a prefix when deleting/clean-up. we see any complexity there?
There was a problem hiding this comment.
This approach seems fragile and prone to problems in the future to me. When we create the dest why not init there?
There was a dependency issue while doing this in table.c file. But, I got the point. Will re-visit this.
bgpd/bgp_route.c
Outdated
| } | ||
| } | ||
|
|
||
| /* Show BGP path info hash keys */ |
There was a problem hiding this comment.
I don't understand the purpose of this new DEFUN. Can you elaborate to me what this is needed and why it's different than just showing the prefix with it's paths?
There was a problem hiding this comment.
I don't understand the purpose of this new DEFUN. Can you elaborate to me what this is needed and why it's different than just showing the prefix with it's paths?
Ack. I thought of adding the keys for debugging purpose. But, we can display the same in the existing show command itself.
krishna-samy
force-pushed
the
krishna/bgp-path-lookup
branch
from
b2d8f2c to
d6490dc
Compare
|
Below is the memory footprint with the per-table hash implementation Memory without any hash: Memory per table hash: ~7% increase with per-table hash CPU performance is improved by 50-60% as shared in the above comments. |
I don't know what these blocks are, but 3 very different numbers of "Ordinary Blocks", and yet the actual memory usage seems close to identical -- seems a bit fishy. |
krishna-samy
force-pushed
the
krishna/bgp-path-lookup
branch
from
d6490dc to
3ef3edd
Compare
Problem: -------- BGP path lookup currently uses O(N) linear search through the list (dest->info) for every incoming route update. In high-ECMP scenarios, each update requires iterating through the entire path list to check if a path from that peer already exists. This becomes a severe performance bottleneck in data center environments with high ECMP during large-scale route updates churn. Solution: --------- Implement a path_info hash per table using typesafe hash. Use the same in bgp_update() for efficient lookup. This hash lookup improves CPU overhead upto ~60% Signed-off-by: Krishnasamy R <[email protected]> Signed-off-by: Krishnasamy <[email protected]>
krishna-samy
force-pushed
the
krishna/bgp-path-lookup
branch
from
3ef3edd to
b45c53b
Compare
I think the comment should have been bit more clear. Just updated now. |
|
I'm good with adding hashing, I didn't want to gate this change. I was just noticing that the numbers didn't add up :) |
choppsv1
left a comment
choppsv1
left a comment
There was a problem hiding this comment.
hashing for lookups, good idea.
4 participants
Problem:
BGP path lookup currently uses O(N) linear search through the list (dest->info) for every incoming route update. In high-ECMP scenarios, each update requires iterating through the entire path list to check if a path from that peer already exists.
This becomes a severe performance bottleneck in data center environments with high ECMP during large-scale route updates churn.
Solution:
Implement a path_info hash per table using typesafe hash. Use the same in bgp_update() for efficient lookup.
This hash lookup improves CPU overhead upto ~60%