-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ipsec: cache xfrm state list #32588
ipsec: cache xfrm state list #32588
Conversation
d4637f1
to
b90b538
Compare
b90b538
to
f5dd892
Compare
f5dd892
to
76a9b0b
Compare
/test |
4ce9faf
to
0bb20b5
Compare
/test |
@pchaigno I wonder which CI test runs IPSec privileged test?
🤷 |
But it prints tests="0" for every single packages, doesn't it? I'm not sure we can rely on that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Couple questions below, but nothing I believe is blocking.
0bb20b5
to
522c126
Compare
@marseel @pchaigno I originally reported this issue and I've been playing around with a patch on my side also. Instead of calling xfrm state list here and looping until we find a match, can we just use I haven't tested the impact of this on the GC on a large cluster however, but wondering why we wouldn't want to do this instead of the looping logic? I'm a bit wary of caching the states in general just because it feels like troubleshooting issues with the cache in the future may be difficult or hard to find. |
@sjdot I do agree that we should use While
I would say adding caching should be safer than refactoring that part (also safer for backports), as long as we ensure we don't have other calls that are modifying xfrmState with CI test. |
Thanks @marseel Yeah in my patch there are still cases where I have to call list and pass to the functions you mentioned (e.g. a state that needs to be deleted an re-added), I've also not handled the migration of old state so I agree back porting is painful/difficult. I see the usefulness of the caching given this, I'm just nervous given our long history of very painful ipsec issues. Given it's a pretty significant change, do you think it would be feasible to put this behind a feature flag initially? I would like the ability to turn this off if we hit an issue we suspect might be related to be able to quickly verify if this is the cause or not. |
Yup, I think that definitely makes sense. |
522c126
to
ccf72a2
Compare
Reduces GC CPU usage and memory allocations coming from XfrmStateList.
To ensure we have up-to-date cache, wrap all XfrmState related
functions inside cache, which is invalidated whenever XfrmState changes.
This is follow-up to #32577
While that PR averages out CPU usage over time, in large cluster 100+
nodes amount of allocations coming from netlink.XfrmStateList() is high
due to backgroundSync where we usually don't change any Xfrm states.
This becomes more and more expensive as number of nodes increases.
Some pprofs/GC CPU time graphs.
Before:
Number of alloc objects:
Total GC time for 100 nodes:
After:
Number of alloc objects:
Total GC time for 100 nodes:
This would become even more important for larger clusters, as XfrmStateList allocations in backgroundSync are essentially O(n^2).