Reorg to factor mac entries setup and add a max entries test #2587
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
olsajiri
added
the
release-note/minor
mtardy
reviewed
Jun 20, 2024
There was a problem hiding this comment.
thanks! Yeah the hasMaps make sense, especially for fdinstall. In the singleKprobe part it looks a bit weird that we copy the info in this when it's available on the genericKprobe directly but overall with the multi_kprobe, it's better than this lonely fdinstall bool.
Thanks a lot for the test, I wanted to write something similar to check that those memory patch aren't broken! That's really cool :)
maybe we could document in this hasMap struct which are per kprobe and which are per sensor?
pkg/sensors/tracing/generickprobe.go
Outdated
| @@ -272,14 +272,12 @@ func filterMaps(load *program.Program, pinPath string, kprobeEntry *genericKprob | |||
| return maps | |||
| } | |||
|
|
|||
| func createMultiKprobeSensor(sensorPath, policyName string, multiIDs []idtable.EntryID, enableFDInstall bool) ([]*program.Program, []*program.Map, error) { | |||
| func createMultiKprobeSensor(sensorPath, policyName string, multiIDs []idtable.EntryID, has *hasMaps) ([]*program.Program, []*program.Map, error) { | |||
There was a problem hiding this comment.
nit: maybe I would pass this by value for clarity that it's read only
pkg/sensors/tracing/generickprobe.go
Outdated
Comment on lines
563
to
574
| has.fdInstall = has.fdInstall || selectorsHaveFDInstall(kprobe.Selectors) | ||
| if has.fdInstall { | ||
| break | ||
| } |
There was a problem hiding this comment.
you could simplify it, we can always modify if this gets more complicated later
Suggested change
| has.fdInstall = has.fdInstall || selectorsHaveFDInstall(kprobe.Selectors) | |
| if has.fdInstall { | |
| break | |
| } | |
| if selectorsHaveFDInstall(kprobe.Selectors) { | |
| has.fdInstall = true | |
| break | |
| } |
or
Suggested change
| has.fdInstall = has.fdInstall || selectorsHaveFDInstall(kprobe.Selectors) | |
| if has.fdInstall { | |
| break | |
| } | |
| if selectorsHaveFDInstall(kprobe.Selectors) { | |
| return &hasMap{ | |
| fdInstall: true, | |
| } | |
| } |
olsajiri
force-pushed
the
pr/olsajiri/entries
branch
from
40e9d16
to
a7b3199
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mtardy
changed the title
Pass spec pointer directly to createGenericTracepointSensor, it will make following changes easier. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Centralizing maps 'has' config into hasMaps object, which gets initialized before we create the sensor and checked when we create sensor maps. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
olsajiri
force-pushed
the
pr/olsajiri/entries
branch
from
a7b3199
to
20f86f7
Compare
We need enforcer map only when enforcer is configured, so making the map entries by default 1 and resizing it when it's needed. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
olsajiri
force-pushed
the
pr/olsajiri/entries
branch
3 times, most recently
from
24920e3
to
e1b5c16
Compare
Adding max entries test for all maps that are resized based on configuration. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
olsajiri
force-pushed
the
pr/olsajiri/entries
branch
from
e1b5c16
to
6f50e69
Compare
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.
centralizing map entries configuration and adding tests