-
Notifications
You must be signed in to change notification settings - Fork 431
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
Optimize init program #3923
Optimize init program #3923
Conversation
2176242
to
838f64b
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.
That's great!
I've done the first pass and did some questions.
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 will run this full branch on my cluster before giving a +1. Had some questions too.
Have to note on the filldir64 case in particular: I believe the case there isn't this optimization in particular. Note that we have a quick terminating conditional based on a kprobe context argument (the inode_number == 0 check). We could have just as well moved it to the head of the function before the init,trace,submit function trio, and had just the same effect.
d27f636
to
c2ef413
Compare
It passed locally, I can't see why it failed when triggered by the action: https://github.com/aquasecurity/tracee/actions/runs/8360692120/job/22887161311. Anyhoo, I'm gonna replay this test in Github. |
b8cb2ee
to
27f803f
Compare
init_tailcall_program_data() function was missing the initialization of proc_info and args_buf - add them
socket_dup event depends on the arguments of the matching dup syscall. Add this missing dependency.
27f803f
to
0422a39
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.
Great.
We currently initialize all task context related fields on the beginning of our BPF programs. Reading all the relevant fields takes some CPU cycles although these fields are not always used. In many cases, for example when filters are applied, the event is not sent to userspace and is just dropped without any side effects. Reading only the essential fields and leaving all other fields initialization to the submit stage will avoid wasting these cycles, and will also allow us to perform some of the event enrichment in userspace instead (not part of this PR). To do this change, we have to remove the matched_policies which were cached in task_info since we can't check if "context changed" anymore. Considering that the average user will only use one or two scope filters (e.g. container id or binary name), computing the scope on every run is not a big overhead - running BPF statistics with and without the change verified this is indeed the case, and with the change the performance is even slightly better. Although this change does not introduce a visible performance gain for most of the events, it is clear for some other specific event that it does. For example, the hidden_inode event which attach a program to the filldir64 function has a 50% performance gain.
0422a39
to
fe5f097
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.
Performance test turned out good, let's merge.
1. Explain what the PR does
2. Explain how to test it
3. Other comments