libbpf-tools: fix readahead, support v5.10+ kernel #3253
Conversation
ethercflow
requested review from
4ast,
brendangregg,
drzaeus77,
goldshtn and
yonghong-song
as code owners
There was a problem hiding this comment.
It would probably be cleaner (and less code churn) to leave single BPF_KPROBE and BPF_KRETPROBE program in BPF code (say for the latest variant of function name: do_page_cache_ra), but in runtime using explicit bpf_program__attach_kprobe() call to attach it. At that point, you can override the actual kernel function to attach to. WDYT?
Also, did the readahead start-up time become significantly slower due to ksyms index building? I was wondering if we should provide a simple way to find a single kernel symbol without parsing and remembering all symbols. That should speed up use cases like this, because you'll be parse /proc/kallsyms one line at a time and discarding lines that don't match. And once you found a match, you can finish early. Can you please give it a try?
That sounds cool, the current way of writing is really frustrating. I'll look at
Sure, I was wondering whether fallback can be achieved through the error code of |
I don't know if hard-coding ENOENT is specific enough. But I guess you can just try attaching one, if that fails (with any error), try to attach another one. If that one fails, then report error. It's not as user-friendly as checking existence of ksymbol, but would work and would be pretty fast. |
ethercflow
force-pushed
the
bugfix
branch
3 times, most recently
from
d5b3d1f
to
d3aa5b2
Compare
@anakryiko Fixed, PTAL, thanks! |
Hm.. sorry, I didn't realize that we were using fentry/fexit, not kprobe/kretprobe. It would be nice to stick fentry/fexit throughout. Let's use the same approach, but instead of doing bpf_program__attach_kprobe() and falling back to attach for another function, we can do bpf_program__set_attach_target() for one function, and if that fails, we can try bpf_program__set_attach_target() for another one. If both fail -- we are screwed and kernel doesn't support BTF or neither function is present. I think it's actually even cleaner overall, because you can still use skeleton's attach method. You'll just have separate skeleton_open, set_attach_target with fallback, skeleton_load, and then skeleton_attach. How about that? |
Yeah, I like this suggestion, it looks clear and natural with skeletion, I'll changed to that. Thanks! :) |
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
@anakryiko Update, PTAL, thanks! |
| if (!err) | ||
| return 0; | ||
|
|
||
| fprintf(stderr, "failed to set attach target to %s: %s\n", |
| return 0; | ||
|
|
||
| fprintf(stderr, "failed to set attach target to %s: %s\n", | ||
| bpf_program__section_name(prog), strerror(-err)); |
There was a problem hiding this comment.
please use bpf_program__name() instead of bpf_program__section_name()
| err = readahead__set_attach_target(obj->progs.do_page_cache_ra); | ||
| if (err) | ||
| goto cleanup; | ||
| err = readahead__set_attach_target(obj->progs.do_page_cache_ra_ret); | ||
| if (err) | ||
| goto cleanup; |
There was a problem hiding this comment.
this looks pretty clean, I like it!
|
[buildbot, test this please] |
1 similar comment
|
[buildbot, test this please] |
|
@ethercflow I missed that there are some minor changes required. Please do a followup. Thanks! |
@yonghong-song Fixed, PTAL, thanks! :) |
Fix readahead, support 5.10+ kernel, mentioned in #3224
Signed-off-by: Wenbo Zhang ethercflow@gmail.com