| Message ID | 20230512103354.48374-5-quentin@isovalent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpftool: Fix skeletons compilation for older kernels | expand |
On Fri, May 12, 2023 at 11:33:54AM +0100, Quentin Monnet wrote: > From: Alexander Lobakin <alobakin@pm.me> > > Fix the following error when building bpftool: > > CLANG profiler.bpf.o > CLANG pid_iter.bpf.o > skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value' > __uint(value_size, sizeof(struct bpf_perf_event_value)); > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint' > tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value' > struct bpf_perf_event_value; > ^ > > struct bpf_perf_event_value is being used in the kernel only when > CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then. hi, when I switch off CONFIG_BPF_EVENTS the bpftool build fails for me with missing BTF error: GEN vmlinux.h libbpf: failed to find '.BTF' ELF section in /home/jolsa/kernel/linux-qemu/vmlinux Error: failed to load BTF from /home/jolsa/kernel/linux-qemu/vmlinux: No data available make: *** [Makefile:208: vmlinux.h] Error 195 make: *** Deleting file 'vmlinux.h' so I wonder you need to care about bpf_perf_event_value in that case jirka > Define struct bpf_perf_event_value___local with the > `preserve_access_index` attribute inside the pid_iter BPF prog to > allow compiling on any configs. It is a full mirror of a UAPI > structure, so is compatible both with and w/o CO-RE. > bpf_perf_event_read_value() requires a pointer of the original type, > so a cast is needed. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > --- > tools/bpf/bpftool/skeleton/profiler.bpf.c | 27 ++++++++++++++--------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c > index ce5b65e07ab1..2f80edc682f1 100644 > --- a/tools/bpf/bpftool/skeleton/profiler.bpf.c > +++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c > @@ -4,6 +4,12 @@ > #include <bpf/bpf_helpers.h> > #include <bpf/bpf_tracing.h> > > +struct bpf_perf_event_value___local { > + __u64 counter; > + __u64 enabled; > + __u64 running; > +} __attribute__((preserve_access_index)); > + > /* map of perf event fds, num_cpu * num_metric entries */ > struct { > __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); > @@ -15,14 +21,14 @@ struct { > struct { > __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); > __uint(key_size, sizeof(u32)); > - __uint(value_size, sizeof(struct bpf_perf_event_value)); > + __uint(value_size, sizeof(struct bpf_perf_event_value___local)); > } fentry_readings SEC(".maps"); > > /* accumulated readings */ > struct { > __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); > __uint(key_size, sizeof(u32)); > - __uint(value_size, sizeof(struct bpf_perf_event_value)); > + __uint(value_size, sizeof(struct bpf_perf_event_value___local)); > } accum_readings SEC(".maps"); > > /* sample counts, one per cpu */ > @@ -39,7 +45,7 @@ const volatile __u32 num_metric = 1; > SEC("fentry/XXX") > int BPF_PROG(fentry_XXX) > { > - struct bpf_perf_event_value *ptrs[MAX_NUM_MATRICS]; > + struct bpf_perf_event_value___local *ptrs[MAX_NUM_MATRICS]; > u32 key = bpf_get_smp_processor_id(); > u32 i; > > @@ -53,10 +59,10 @@ int BPF_PROG(fentry_XXX) > } > > for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) { > - struct bpf_perf_event_value reading; > + struct bpf_perf_event_value___local reading; > int err; > > - err = bpf_perf_event_read_value(&events, key, &reading, > + err = bpf_perf_event_read_value(&events, key, (void *)&reading, > sizeof(reading)); > if (err) > return 0; > @@ -68,14 +74,14 @@ int BPF_PROG(fentry_XXX) > } > > static inline void > -fexit_update_maps(u32 id, struct bpf_perf_event_value *after) > +fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after) > { > - struct bpf_perf_event_value *before, diff; > + struct bpf_perf_event_value___local *before, diff; > > before = bpf_map_lookup_elem(&fentry_readings, &id); > /* only account samples with a valid fentry_reading */ > if (before && before->counter) { > - struct bpf_perf_event_value *accum; > + struct bpf_perf_event_value___local *accum; > > diff.counter = after->counter - before->counter; > diff.enabled = after->enabled - before->enabled; > @@ -93,7 +99,7 @@ fexit_update_maps(u32 id, struct bpf_perf_event_value *after) > SEC("fexit/XXX") > int BPF_PROG(fexit_XXX) > { > - struct bpf_perf_event_value readings[MAX_NUM_MATRICS]; > + struct bpf_perf_event_value___local readings[MAX_NUM_MATRICS]; > u32 cpu = bpf_get_smp_processor_id(); > u32 i, zero = 0; > int err; > @@ -102,7 +108,8 @@ int BPF_PROG(fexit_XXX) > /* read all events before updating the maps, to reduce error */ > for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) { > err = bpf_perf_event_read_value(&events, cpu + i * num_cpu, > - readings + i, sizeof(*readings)); > + (void *)(readings + i), > + sizeof(*readings)); > if (err) > return 0; > } > -- > 2.34.1 >
2023-05-12 14:59 UTC+0200 ~ Jiri Olsa <olsajiri@gmail.com> > On Fri, May 12, 2023 at 11:33:54AM +0100, Quentin Monnet wrote: >> From: Alexander Lobakin <alobakin@pm.me> >> >> Fix the following error when building bpftool: >> >> CLANG profiler.bpf.o >> CLANG pid_iter.bpf.o >> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value' >> __uint(value_size, sizeof(struct bpf_perf_event_value)); >> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint' >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value' >> struct bpf_perf_event_value; >> ^ >> >> struct bpf_perf_event_value is being used in the kernel only when >> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then. > > hi, > when I switch off CONFIG_BPF_EVENTS the bpftool build fails for me > with missing BTF error: > > GEN vmlinux.h > libbpf: failed to find '.BTF' ELF section in /home/jolsa/kernel/linux-qemu/vmlinux > Error: failed to load BTF from /home/jolsa/kernel/linux-qemu/vmlinux: No data available > make: *** [Makefile:208: vmlinux.h] Error 195 > make: *** Deleting file 'vmlinux.h' > > so I wonder you need to care about bpf_perf_event_value > in that case > > jirka Thanks for testing, Jiri! As I understand, this is the vmlinux.h generation failing, not the compilation from the updated skeletons, so I'm not sure this is related to this patch, could be an existing bug. Have you tried the same build without the patches, by any chance? I'll try to reproduce on my side to try and figure out why libbpf's bpf_parse_elf() fails to find the .BTF section. Quentin
On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@isovalent.com> wrote: > > From: Alexander Lobakin <alobakin@pm.me> > > Fix the following error when building bpftool: > > CLANG profiler.bpf.o > CLANG pid_iter.bpf.o > skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value' > __uint(value_size, sizeof(struct bpf_perf_event_value)); > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint' > tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value' > struct bpf_perf_event_value; > ^ > > struct bpf_perf_event_value is being used in the kernel only when > CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then. > Define struct bpf_perf_event_value___local with the > `preserve_access_index` attribute inside the pid_iter BPF prog to > allow compiling on any configs. It is a full mirror of a UAPI > structure, so is compatible both with and w/o CO-RE. > bpf_perf_event_read_value() requires a pointer of the original type, > so a cast is needed. > > Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > --- What's the point of using vmlinux.h at all if we redefine every single type? bpf_perf_event_value is part of BPF UAPI, so if we included linux/bpf.h header we'd get it. This feels a bit split-brained. We either drop vmlinux.h completely and use UAPI headers + CO-RE-relocatable definitions of internal types, or we make sure that vmlinux.h does work (e.g., by pre-checking in a very small version of it). Both using vmlinux.h and not relying on it having necessary types seems like the worst of both worlds?... Quentin, can you see if you can come up with some simple way to use vmlinux.h if building from inside kernel repo, but using a minimized vmlinux.h generated using `bpftool gen min_core_btf` when building from Github mirror? Sync script could also generate this minimal vmlinux.h automatically, presumably? > tools/bpf/bpftool/skeleton/profiler.bpf.c | 27 ++++++++++++++--------- > 1 file changed, 17 insertions(+), 10 deletions(-) > [...]
2023-05-16 14:30 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> From: Alexander Lobakin <alobakin@pm.me> >> >> Fix the following error when building bpftool: >> >> CLANG profiler.bpf.o >> CLANG pid_iter.bpf.o >> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value' >> __uint(value_size, sizeof(struct bpf_perf_event_value)); >> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint' >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value' >> struct bpf_perf_event_value; >> ^ >> >> struct bpf_perf_event_value is being used in the kernel only when >> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then. >> Define struct bpf_perf_event_value___local with the >> `preserve_access_index` attribute inside the pid_iter BPF prog to >> allow compiling on any configs. It is a full mirror of a UAPI >> structure, so is compatible both with and w/o CO-RE. >> bpf_perf_event_read_value() requires a pointer of the original type, >> so a cast is needed. >> >> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> >> Signed-off-by: Alexander Lobakin <alobakin@pm.me> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >> --- > > What's the point of using vmlinux.h at all if we redefine every single > type? bpf_perf_event_value is part of BPF UAPI, so if we included > linux/bpf.h header we'd get it. I gave a quick try at the UAPI header before posting this patch, but it was an Ubuntu box and I got the "asm/types.h not found" error. If I remember correctly, one way to fix this is to have the gcc-multilib, which I'd rather avoid to add as a dependency; or adding the correct include path for x86_64 at least, which I haven't tried for bpftool yet. > > This feels a bit split-brained. We either drop vmlinux.h completely > and use UAPI headers + CO-RE-relocatable definitions of internal > types, or we make sure that vmlinux.h does work (e.g., by pre-checking > in a very small version of it). Both using vmlinux.h and not relying > on it having necessary types seems like the worst of both worlds?... Yeah I do feel like I'm missing something in this set and the approach is not optimal. What do you mean exactly by "pre-checking in a very small version of it"? Checking for the availability of a few types and exit early from the functions if they're missing, because we're assuming we won't have support for the feature? > Quentin, can you see if you can come up with some simple way to use > vmlinux.h if building from inside kernel repo, but using a minimized > vmlinux.h generated using `bpftool gen min_core_btf` when building > from Github mirror? Sync script could also generate this minimal > vmlinux.h automatically, presumably? Sure, I can look into it. But not right now - I'd like to get the current issue, and the (unrelated) LLVM feature detection, sorted before starting on this. Thanks for your feedback! Quentin
On Wed, May 17, 2023 at 8:02 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2023-05-16 14:30 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@isovalent.com> wrote: > >> > >> From: Alexander Lobakin <alobakin@pm.me> > >> > >> Fix the following error when building bpftool: > >> > >> CLANG profiler.bpf.o > >> CLANG pid_iter.bpf.o > >> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value' > >> __uint(value_size, sizeof(struct bpf_perf_event_value)); > >> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint' > >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value' > >> struct bpf_perf_event_value; > >> ^ > >> > >> struct bpf_perf_event_value is being used in the kernel only when > >> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then. > >> Define struct bpf_perf_event_value___local with the > >> `preserve_access_index` attribute inside the pid_iter BPF prog to > >> allow compiling on any configs. It is a full mirror of a UAPI > >> structure, so is compatible both with and w/o CO-RE. > >> bpf_perf_event_read_value() requires a pointer of the original type, > >> so a cast is needed. > >> > >> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command") > >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> > >> Signed-off-by: Alexander Lobakin <alobakin@pm.me> > >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> > >> --- > > > > What's the point of using vmlinux.h at all if we redefine every single > > type? bpf_perf_event_value is part of BPF UAPI, so if we included > > linux/bpf.h header we'd get it. > > I gave a quick try at the UAPI header before posting this patch, but it > was an Ubuntu box and I got the "asm/types.h not found" error. If I > remember correctly, one way to fix this is to have the gcc-multilib, > which I'd rather avoid to add as a dependency; or adding the correct > include path for x86_64 at least, which I haven't tried for bpftool yet. > > > > > This feels a bit split-brained. We either drop vmlinux.h completely > > and use UAPI headers + CO-RE-relocatable definitions of internal > > types, or we make sure that vmlinux.h does work (e.g., by pre-checking > > in a very small version of it). Both using vmlinux.h and not relying > > on it having necessary types seems like the worst of both worlds?... > > Yeah I do feel like I'm missing something in this set and the approach > is not optimal. What do you mean exactly by "pre-checking in a very > small version of it"? Checking for the availability of a few types and > exit early from the functions if they're missing, because we're assuming > we won't have support for the feature? So I was thinking that we'll keep relying on vmlinux BTF and proper Kconfig for bpftool build inside kernel repo, no changes there. But then we can use `bpftool gen min_core_btf` on resulting .bpf.o files to dump only types that are actually used/referenced by bpftool's BPF object files, and then we can generate vmlinux.h from that minimized BTF. I haven't tried it, and I'm sure there will be some hiccups along the way, but that was the idea. > > > Quentin, can you see if you can come up with some simple way to use > > vmlinux.h if building from inside kernel repo, but using a minimized > > vmlinux.h generated using `bpftool gen min_core_btf` when building > > from Github mirror? Sync script could also generate this minimal > > vmlinux.h automatically, presumably? > > Sure, I can look into it. But not right now - I'd like to get the > current issue, and the (unrelated) LLVM feature detection, sorted before > starting on this. Sounds good. In general, your patches look good to me, I was hoping we can avoid unnecessary definitions of UAPI types, but if that doesn't work out of the box, then I'm fine with it. > > Thanks for your feedback! > Quentin
2023-05-12 14:59 UTC+0200 ~ Jiri Olsa <olsajiri@gmail.com> > On Fri, May 12, 2023 at 11:33:54AM +0100, Quentin Monnet wrote: >> From: Alexander Lobakin <alobakin@pm.me> >> >> Fix the following error when building bpftool: >> >> CLANG profiler.bpf.o >> CLANG pid_iter.bpf.o >> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value' >> __uint(value_size, sizeof(struct bpf_perf_event_value)); >> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint' >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value' >> struct bpf_perf_event_value; >> ^ >> >> struct bpf_perf_event_value is being used in the kernel only when >> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then. > > hi, > when I switch off CONFIG_BPF_EVENTS the bpftool build fails for me > with missing BTF error: > > GEN vmlinux.h > libbpf: failed to find '.BTF' ELF section in /home/jolsa/kernel/linux-qemu/vmlinux > Error: failed to load BTF from /home/jolsa/kernel/linux-qemu/vmlinux: No data available > make: *** [Makefile:208: vmlinux.h] Error 195 > make: *** Deleting file 'vmlinux.h' > > so I wonder you need to care about bpf_perf_event_value > in that case > > jirka Coming back to this - I haven't been able to reproduce. I turned CONFIG_BPF_EVENTS off but had the vmlinux.h building fine in my case, and applying the patchset fixes the build for bpftool. Is there any chance your .../vmlinux was not generated correctly? I'll re-submit the series after addressing Yonghong's feedback on the commit log. Thanks, Quentin
diff --git a/tools/bpf/bpftool/skeleton/profiler.bpf.c b/tools/bpf/bpftool/skeleton/profiler.bpf.c index ce5b65e07ab1..2f80edc682f1 100644 --- a/tools/bpf/bpftool/skeleton/profiler.bpf.c +++ b/tools/bpf/bpftool/skeleton/profiler.bpf.c @@ -4,6 +4,12 @@ #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> +struct bpf_perf_event_value___local { + __u64 counter; + __u64 enabled; + __u64 running; +} __attribute__((preserve_access_index)); + /* map of perf event fds, num_cpu * num_metric entries */ struct { __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY); @@ -15,14 +21,14 @@ struct { struct { __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); __uint(key_size, sizeof(u32)); - __uint(value_size, sizeof(struct bpf_perf_event_value)); + __uint(value_size, sizeof(struct bpf_perf_event_value___local)); } fentry_readings SEC(".maps"); /* accumulated readings */ struct { __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); __uint(key_size, sizeof(u32)); - __uint(value_size, sizeof(struct bpf_perf_event_value)); + __uint(value_size, sizeof(struct bpf_perf_event_value___local)); } accum_readings SEC(".maps"); /* sample counts, one per cpu */ @@ -39,7 +45,7 @@ const volatile __u32 num_metric = 1; SEC("fentry/XXX") int BPF_PROG(fentry_XXX) { - struct bpf_perf_event_value *ptrs[MAX_NUM_MATRICS]; + struct bpf_perf_event_value___local *ptrs[MAX_NUM_MATRICS]; u32 key = bpf_get_smp_processor_id(); u32 i; @@ -53,10 +59,10 @@ int BPF_PROG(fentry_XXX) } for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) { - struct bpf_perf_event_value reading; + struct bpf_perf_event_value___local reading; int err; - err = bpf_perf_event_read_value(&events, key, &reading, + err = bpf_perf_event_read_value(&events, key, (void *)&reading, sizeof(reading)); if (err) return 0; @@ -68,14 +74,14 @@ int BPF_PROG(fentry_XXX) } static inline void -fexit_update_maps(u32 id, struct bpf_perf_event_value *after) +fexit_update_maps(u32 id, struct bpf_perf_event_value___local *after) { - struct bpf_perf_event_value *before, diff; + struct bpf_perf_event_value___local *before, diff; before = bpf_map_lookup_elem(&fentry_readings, &id); /* only account samples with a valid fentry_reading */ if (before && before->counter) { - struct bpf_perf_event_value *accum; + struct bpf_perf_event_value___local *accum; diff.counter = after->counter - before->counter; diff.enabled = after->enabled - before->enabled; @@ -93,7 +99,7 @@ fexit_update_maps(u32 id, struct bpf_perf_event_value *after) SEC("fexit/XXX") int BPF_PROG(fexit_XXX) { - struct bpf_perf_event_value readings[MAX_NUM_MATRICS]; + struct bpf_perf_event_value___local readings[MAX_NUM_MATRICS]; u32 cpu = bpf_get_smp_processor_id(); u32 i, zero = 0; int err; @@ -102,7 +108,8 @@ int BPF_PROG(fexit_XXX) /* read all events before updating the maps, to reduce error */ for (i = 0; i < num_metric && i < MAX_NUM_MATRICS; i++) { err = bpf_perf_event_read_value(&events, cpu + i * num_cpu, - readings + i, sizeof(*readings)); + (void *)(readings + i), + sizeof(*readings)); if (err) return 0; }