Add fallback to no kretprobes #582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #582 +/- ##
==========================================
+ Coverage 79.26% 79.64% +0.37%
==========================================
Files 70 70
Lines 5907 5909 +2
==========================================
+ Hits 4682 4706 +24
+ Misses 1001 978 -23
- Partials 224 225 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
grcevski
commented
Jan 26, 2024
pkg/internal/ebpf/instrumenter.go
Outdated
| // We should make RetprobeMaxActive configurable when we make the num concurrent requests configurable | ||
| // By default it sets itself to at least 10, but at most 2 * num cpus, which is low for something like tcp_recvmsg. Max value is 4096. | ||
| // https://elixir.bootlin.com/linux/v5.19/source/kernel/kprobes.c#L2202 | ||
| kp, err := link.Kretprobe(funcName, programs.End, &link.KprobeOptions{RetprobeMaxActive: 1024}) |
There was a problem hiding this comment.
Where other eBPF projects set this is either 1024 or the max. Let's start with 1024 and see how this works.
mariomac
approved these changes
Jan 29, 2024
|
Thanks Mario! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We used to track kprobe events by using a socket filter, which was difficult to work with for the cases we cared about. It was not easy to find matching PIDs or read more than 250 bytes. We replaced the socket filter by kprobes/kretprobes on tcp_sendmsg and tcp_recvmsg.
However, as per the bug report discussion in #573, essentially kprobes can be discarded if the events are taking too long, which makes the kprobe on tcp_recvmsg a prime candidate in high network workload scenarios.
To fix this issue, with this PR I'm bringing back the socket filter, targeted only for creating a fallback information for the HTTP requests. It doesn't do the full work as before, it only captures some essential information to be used if the kretprobe on tcp_recvmsg was eliminated.
I should also mention that we have other kretprobes, but they are on sock_alloc and accept4, those will typically be very fast and unlikely to be cancelled, unlike receiving of network buffers.
To test this scenario, I manually removed the code from the retprobe on tcp_recvmsg and tested with Apache2 to see if we still see the routes.