-
Notifications
You must be signed in to change notification settings - Fork 169
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
wip: new(driver/modern_bpf): home-made bpf_loop for sendmmsg and recvmmsg. #2233
base: master
Are you sure you want to change the base?
Conversation
/milestone TBD |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I want to hear @Andreagit97 and @Molter73 opinions on this one :) |
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2233 +/- ##
=======================================
Coverage 75.09% 75.09%
=======================================
Files 276 276
Lines 34391 34391
Branches 5927 5927
=======================================
Hits 25826 25826
Misses 8565 8565
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
X64 kernel testing matrix
ARM64 kernel testing matrix
|
Need to understand what broke e2e-tests. |
dd30172
to
5664af4
Compare
With this one weird trick, bpf hates us! Signed-off-by: Federico Di Pierro <[email protected]>
5664af4
to
8e15871
Compare
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.
The approach seems great! thank you! if we want to avoid macro X we can just use a single ebpf program in tail call and save the state in a PER-CPU map/array so that each tailed called prog can see the iteration number. In the end, it should change almost nothing, it would be probably just easier to debug
If we create a per-cpu map, we could also avoid to share |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area driver-modern-bpf
Does this PR require a change in the driver versions?
What this PR does / why we need it:
For sendmmsg and recvmmsg in modern_bpf probe we could not use
bpf_loop
helper because it caused verifier issues on kernels prior to 5.13 (see #2027 (comment)).Therefore we used a loop up to 16; then we noticed that 16 was too high, thus we lowered the limit to 8 (600fefb).
This means we can only read first 8 messages sent through
sendmmsg
andrecvmmsg
.This PR's scope is to increase the limit up to 256 (in the first proposed draft, i limit it to 64).
The idea is to build an
X_MACRO
that let us easily chain tail-calls.EBPF tail call limit is MAX_TAIL_CALL_CNT, ie 32 on older kernels, and 33 nowadays. For now, i capped the implementation to 8 (and each tail-call loops 8 times, this 64 total).
Note also that to make it a little bit less verbose, i could also create a new header with all the macros and share them between the 2 source files, since they are basically identical (naming aside). I decided to keep it as easy as possible, but it is a possibility (and it would be more future proof since we would only have a single place to update if needed).
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Putting it in
wip
to allow a discussion.THIS IS NOT FOR 0.20.0.
Does this PR introduce a user-facing change?: