-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
libbpf-tools: add CO-RE xfsslower #2806
Conversation
Hi @anakryiko , I want to try
Would you please help me to find the faults in the code? Thank you. |
I've
|
Right, xfs is built as a module, that's what I suspected. BTF is generated and supported only for vmlinux itself right now, not for kernel modules, unfortunately. So to make fentry/fexit work with them you'll need to build-in XFS support. |
Thank you @anakryiko , I just tried with build-in XFS, but still get the same errror:
To use
|
You have pahole 1.15, isn't that right? fentry/fexit relies on func infos in BTF, which are produced starting from pahole 1.16, can you please try upgrading? |
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.
implementation looks good, few suggestions only
Thank you @anakryiko , I updated to 1.16 at host then make clean && rebuild kernel with gcc 8.3.0, and restart qemu to load the recompiled kernel. But still met that error. Did I miss anything else?
|
So I have bad news for you w.r.t. fentry/fexit usage here. xfs_file_read_iter is static function, and those functions are not recorded (at least right now) in BTF. Which means you can't attach to them with fentry/fexit programs. Your best bet for now is to stick to kprobe, probably. |
Get it, I'll change this to |
Hi @anakryiko , are I tried SEC("kprobe/acct_collect")
int BPF_KPROBE(kprobe__acct_collect, long exit_code, int group_dead)
{
} with libbpf/libbpf@9a424be, but got some warning and error at compile time: BTW, from
so if the functions are from kernel modules or defined as static. I need to use as bcc's way like: #ifdef __BCC__
int kprobe__acct_collect(struct pt_regs *ctx, long exit_code, int group_dead) instead of CO-RE right? Thank you. |
Wenbo, right, once libbpf is updated (#2814), BPF_KPROBE and PT_REGS_PARM macros should be available. But there is a bit of complication due to using -target bpf, which loses track of host architecture. I forgot about that, given kernel selftests solved that with extra |
As for the other part of your question. You are conflating few things. When talking about kprobes, we can talk about BCC and libbpf version only. CO-RE is, in the most strict sense, only about field relocations (so all those BPF_CORE_READ macro). Then fentry/fexit are another type of BPF programs, can work without using CO-RE relocations, but in practice those two are used together most frequently. So for your use case, you need libbpf-style kprobes. I'd recomment using BPF_KPROBE, but you can go with:
BPF_KPROBE conveniently hides those translations from you, nothing more. We can call this libbpf-style kprobes. |
Thank you @anakryiko so much, I did think that the clang 's precompiler might handle
It is a bad habit to speculate on the principle, I should throw my doubts out and ask. :( I think I misunderstood and I have a confusion about:
As
with Does it mean I need to use another libbpf's APIs in userspace like libbcc did? (I think I only changed from |
Thank you @anakryiko , I tested on x86_64, it works for me. No warnings and errors at compile time. But got runtime error:
Does it related to #2806 (comment)? |
@ethercflow, BTF is used for many things. At it's essence it's just a type information available form kernel at runtime. BTF is used for CO-RE relocations, of course. But it's also used for fentry/fexit to find kernel's function type definiton. It's also used for .struct_ops, but that's for another day. My point is that BTF != CO-RE, BTF is more generally used. Now, you can't use fentry/fexit without BTF. Which means you are stuck with kprobe. If you further need to use CO-RE with that kprobe with types defined in a module -- then again, you can't do that. It all comes down to BTF availability for functions and types you need from kernel. In your case, seems like you only care about struct kiocb and struct file, both should be in kernel. So it seems like kprobe + CO-RE will work for your use-case, no? So please try that approach. |
@anakryiko Thank you for the details, I think I understand this now.
Please help me check my understanding of this passage:
Thank you, I tried with
Then I replace xfsslower.bpf.c's #include "vmlinux.h"
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_core_read.h>
#include <bpf/bpf_tracing.h>
SEC("kprobe/acct_collect")
int BPF_KPROBE(kprobe__acct_collect, long exit_code, int group_dead)
{
bpf_printk("%ld,%d\n", exit_code, group_dead);
return 0;
}
char LICENSE[] SEC("license") = "GPL"; got similar error (with -v):
What's the meaning of this error and how to fix this? |
You're welcome! Glad it was helpful.
Correct.
Correct again.
Ok, I think this is actually a bug in libbpf. It's trying to do relocation against struct pt_regs forward declaration and fails. I'm gonna work on a fix, but meanwhile to unblock yourself you can add the following
This will disable |
I'll keep going after work, thanks. :) |
alright, should be fixed as part of this series: https://patchwork.ozlabs.org/project/netdev/list/?series=164156&state=* |
…ocation When finding target type candidates, ignore forward declarations, functions, and other named types of incompatible kind. Not doing this can cause false errors. See [0] for one such case (due to struct pt_regs forward declaration). [0] iovisor/bcc#2806 (comment) Fixes: ddc7c3042614 ("libbpf: implement BPF CO-RE offset relocation algorithm") Reported-by: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Martin KaFai Lau <kafai@fb.com> Link: https://lore.kernel.org/bpf/20200313172336.1879637-3-andriin@fb.com
99133fa
to
4239383
Compare
…ocation When finding target type candidates, ignore forward declarations, functions, and other named types of incompatible kind. Not doing this can cause false errors. See [0] for one such case (due to struct pt_regs forward declaration). [0] iovisor/bcc#2806 (comment) Fixes: ddc7c30 ("libbpf: implement BPF CO-RE offset relocation algorithm") Reported-by: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Martin KaFai Lau <kafai@fb.com> Link: https://lore.kernel.org/bpf/20200313172336.1879637-3-andriin@fb.com
4239383
to
52084e6
Compare
Hi @anakryiko , the current PR with
works as expected, PTAL. ➜ libbpf-tools git:(master) ✗ ./xfsslower -m 0
Tracing XFS operations... Hit Ctrl-C to end.
TIME COMM PID T BYTES OFF_KB LAT(ms) FILENAME
01:44:27 qemu-system-x8 24565 W 4096 553672 0.05 stretch.img
01:44:27 qemu-system-x8 24565 W 4096 167248 0.02 stretch.img
01:44:27 qemu-system-x8 24565 W 4096 12255028 0.02 stretch.img
01:44:33 qemu-system-x8 24565 F LL_MAX 0 0.47 stretch.img
01:44:33 qemu-system-x8 24565 W 4096 19009300 0.05 stretch.img
01:44:33 qemu-system-x8 24565 F LL_MAX 0 0.09 stretch.img
01:44:50 qemu-system-x8 24565 W 16384 19009304 0.08 stretch.img
01:44:50 qemu-system-x8 24565 F LL_MAX 0 0.17 stretch.img
01:44:50 qemu-system-x8 24565 W 4096 19009320 0.02 stretch.img
01:44:50 qemu-system-x8 24565 F LL_MAX 0 0.10 stretch.img
01:45:00 qemu-system-x8 24565 W 16384 19009324 0.07 stretch.img
01:45:00 qemu-system-x8 24565 F LL_MAX 0 0.16 stretch.img
01:45:00 qemu-system-x8 24565 W 4096 19009340 0.02 stretch.img
01:45:00 qemu-system-x8 24565 F LL_MAX 0 0.09 stretch.img
01:45:15 qemu-system-x8 24565 W 4096 553672 0.05 stretch.img
01:45:15 qemu-system-x8 24565 W 4096 167248 0.02 stretch.img
01:45:15 qemu-system-x8 24565 W 4096 12255028 0.02 stretch.img
01:45:20 qemu-system-x8 24565 W 4096 2097292 0.05 stretch.img
01:45:20 qemu-system-x8 24565 W 4096 2097444 0.02 stretch.img
01:45:20 qemu-system-x8 24565 W 4096 2097452 0.02 stretch.img
01:45:21 qemu-system-x8 24565 F LL_MAX 0 0.37 stretch.img
01:45:21 qemu-system-x8 24565 W 4096 19009344 0.04 stretch.img
01:45:21 qemu-system-x8 24565 F LL_MAX 0 0.09 stretch.img
01:46:00 qemu-system-x8 24565 W 16384 19009348 0.10 stretch.img
01:46:00 qemu-system-x8 24565 F LL_MAX 0 0.17 stretch.img
01:46:00 qemu-system-x8 24565 W 4096 19009364 0.02 stretch.img
01:46:00 qemu-system-x8 24565 F LL_MAX 0 0.09 stretch.img With a little regret as https://patchwork.ozlabs.org/patch/1211861/'s state is still BTW, I found another two issues:
#define NULL 0
I found qs = BPF_CORE_READ(pidstatp, fp, f_path.dentry, d_name); will be rejected by verifier.
The full log: xfsslower_log.txt Is this behavior as expected? |
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.
yes, NULL is not enum, so you'll have to re-define it (though I guess we can add that to vmlinux as well, I suppose, I'll think about that).
Looks great, few more comments to make this perfect, but I think it's almost ready to land.
…ocation When finding target type candidates, ignore forward declarations, functions, and other named types of incompatible kind. Not doing this can cause false errors. See [0] for one such case (due to struct pt_regs forward declaration). [0] iovisor/bcc#2806 (comment) Fixes: ddc7c30 ("libbpf: implement BPF CO-RE offset relocation algorithm") Reported-by: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
…ocation When finding target type candidates, ignore forward declarations, functions, and other named types of incompatible kind. Not doing this can cause false errors. See [0] for one such case (due to struct pt_regs forward declaration). [0] iovisor/bcc#2806 (comment) Fixes: ddc7c3042614 ("libbpf: implement BPF CO-RE offset relocation algorithm") Reported-by: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Martin KaFai Lau <kafai@fb.com> Link: https://lore.kernel.org/bpf/20200313172336.1879637-3-andriin@fb.com
@ethercflow, BPF_KRETPROBE fix is being pulled into BCC in #2823 |
Great! I'll update the PR after work. Thanks! |
707aa37
to
95b4aa5
Compare
Looks great to me, thanks for doing this and helping hammer out remaining bugs! |
95b4aa5
to
e2d39a0
Compare
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
e2d39a0
to
7b52a89
Compare
@yonghong-song @brendangregg Would you please help me to see if it‘s good enough to be merged? After merge I want to submit another PR with |
[buildbot, test this please] |
LGTM. Thanks! |
…ocation When finding target type candidates, ignore forward declarations, functions, and other named types of incompatible kind. Not doing this can cause false errors. See [0] for one such case (due to struct pt_regs forward declaration). [0] iovisor/bcc#2806 (comment) Fixes: ddc7c3042614 ("libbpf: implement BPF CO-RE offset relocation algorithm") Reported-by: Wenbo Zhang <ethercflow@gmail.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Martin KaFai Lau <kafai@fb.com> Link: https://lore.kernel.org/bpf/20200313172336.1879637-3-andriin@fb.com
fsync
's offset tostart
and size toend - start
instead of zero;Signed-off-by: Wenbo Zhang ethercflow@gmail.com