-
Notifications
You must be signed in to change notification settings - Fork 417
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
feat(caps): base ebpf capabilities #4178
feat(caps): base ebpf capabilities #4178
Conversation
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.
I don't see we removed the ebpf ring request from the hot path in the pipeline - shouldn't we do that?
useBaseEbpf := func(cfg config.Config) bool { | ||
return cfg.Output.StackAddresses | ||
} |
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.
Can't we simply add the required bpf capability to the base ring if StackAddresses is selected?
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.
So if in the future we add other features which require a similar approach, we can add the logic here instead of directly coupling to stackaddresses.
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.
Why coupling?
I mean, to expose some method to add more capabilities to the base ring, and then we can remove all the other changes in this PR, and just add the capability here using this method
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.
There already is such a method, but the eBPF caps specifically aren't constant across systems. So I thought initializing a specific bypass for eBPF in the code location where we do the calculation of what are these eBPF caps, would be easier.
About coupling, I meant that this is slightly less coupled, or at least that it makes the intention that this condition coud change in the future more clear, then doing ebpfBase: cfg.Output.StackAddresses
.
Now there's no need, because the EBPF callback automatically skips the capability switch if we include EBPF caps in the base. So its essentially like a mini-bypass. |
Add the ability to add the ebpf capabilities to the base ring, instead of allocating a specific ring. When the feature is enabled, requests for the eBPF ring will seamlessly avoid a ring switch, and the callback will be called as is.
6bd0b39
to
097c25c
Compare
But this method still tries to acquire the capabilities lock in the hot path |
It wouldn't be the first lock acquisition we have inside the pipeline. The cgroups tracking for example does that. In any case, the performance overhead from capability switching was resolved. If there is lock contention because of this, we can reconsider, I'm not sure there will be (seeing as in similar situations so far we haven't). |
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.
Ok, we can continue with this change for now.
In the future, we can rethink how capabilities are handled.
1. Explain what the PR does
6bd0b39 feat(caps): base ebpf capabilities
2. Explain how to test it
Stack addresses should remain working.
3. Other comments