tetragon: split process event tail call #4439
Conversation
olsajiri
force-pushed
the
pr/olsajiri/process
branch
from
a6569d5 to
06cad1d
Compare
olsajiri
added
the
release-note/minor
olsajiri
force-pushed
the
pr/olsajiri/process
branch
from
06cad1d to
df25b1a
Compare
olsajiri
force-pushed
the
pr/olsajiri/process
branch
from
df25b1a to
cde4cba
Compare
bpf/process/generic_calls.h
Outdated
|
|
||
| size = __read_arg_1(ctx, type, orig_off, arg, argm, args); | ||
| if (!size) | ||
| size = __read_arg_2(ctx, type, orig_off, arg, argm, args); |
There was a problem hiding this comment.
Maybe it's worth considering doing this instead:
if is_read_arg_1(type) {
return __read_arg_1(ctx, type, orig_off, arg, argm, args);
} else {
return __read_arg_2(ctx, type, orig_off, arg, argm, args);
}
I suggest this because it makes it more clear that if you handle a new type in __read_arg_1, you also need to update is_read_arg_1. Otherwise, if a developer's preliminary testing is on a modern kernel, they may not discover the connection between these two functions until they run vmtests and see that 4.19 doesn't work as expected.
Also, while returning zero from __read_arg_1 doesn't make sense for success, and therefore is a suitable way to signal that __read_arg_2 should be called, we could avoid making this new meaning/connection by being explicit when checking is_read_arg_1. This way, we would be free to use the zero return value case for some other meaning in the future. (A meaning that applies both to __read_arg_1 and __read_arg_2)
There was a problem hiding this comment.
makes sense, will change, thanks
olsajiri
force-pushed
the
pr/olsajiri/process
branch
from
cde4cba to
ce698c3
Compare
olsajiri
force-pushed
the
pr/olsajiri/process
branch
2 times, most recently
from
de31954 to
c9e6369
Compare
kkourt
left a comment
kkourt
left a comment
There was a problem hiding this comment.
Code looks good to me, thanks!
| if (process == __READ_ARG_2) | ||
| return __read_arg_2(ctx, type, orig_off, arg, argm, args); | ||
|
|
||
| /* .. and the rest of the world */ |
There was a problem hiding this comment.
| /* .. and the rest of the world */ | |
| /* .. and the rest of the world (i.e., process == __READ_ARG_ALL) */ |
bpf/process/generic_calls.h
Outdated
| am = config->arm[index]; | ||
|
|
||
| #ifndef __LARGE_BPF_PROG | ||
| #if defined(GENERIC_KPROBE) || defined(GENERIC_UPROBE) |
There was a problem hiding this comment.
This is missing GENERIC_TRACEPOINT. It might be the reason why the CI is failing after tacking on the latest commit to enable this for tracepoints as well.
Move argument reading code to separate __read_arg function to ease up following changes. Signed-off-by: Jiri Olsa <[email protected]>
Split __read_arg function into 2 functions to help splitting process tail call for small kernels. Signed-off-by: Jiri Olsa <[email protected]>
Split generic_[ku]probe_process_event into two separate tail calls so we have space (there's 4096 instructions limit on 4.19 kernels) for new features in processing event code. Before: generic_kprobe_process_event 4029 insns generic_uprobe_process_event 4029 insns generic_tracepoint_process_event 3878 insns Now: generic_uprobe_process_event 2795 insns generic_uprobe_process_event_2 3147 insns generic_kprobe_process_event 2795 insns generic_kprobe_process_event_2 3147 insns generic_tracepoint_process_event_2 2925 insns generic_tracepoint_process_event 2501 insns Signed-off-by: Jiri Olsa <[email protected]>
olsajiri
force-pushed
the
pr/olsajiri/process
branch
from
c9e6369 to
f113617
Compare
4 participants
Split generic_[ku]probe_process_event into two separate tail calls, so we have space (there's 4096 instructions limit on 4.19 kernels) for new features in processing event code.