| Message ID | 20200519192341.134360-1-andriin@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: convert bpf_iter_test_kern{3,4}.c to define own bpf_iter_meta | expand |
On Tue, May 19, 2020 at 12:23:41PM -0700, Andrii Nakryiko wrote: > b9f4c01f3e0b ("selftest/bpf: Make bpf_iter selftest compilable against old vmlinux.h") > missed the fact that bpf_iter_test_kern{3,4}.c are not just including > bpf_iter_test_kern_common.h and need similar bpf_iter_meta re-definition > explicitly. > > Fixes: b9f4c01f3e0b ("selftest/bpf: Make bpf_iter selftest compilable against old vmlinux.h") > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > .../selftests/bpf/progs/bpf_iter_test_kern3.c | 15 +++++++++++++++ > .../selftests/bpf/progs/bpf_iter_test_kern4.c | 15 +++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c > index 636a00fa074d..13c2c90c835f 100644 > --- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c > @@ -1,10 +1,25 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright (c) 2020 Facebook */ > +#define bpf_iter_meta bpf_iter_meta___not_used > +#define bpf_iter__task bpf_iter__task___not_used > #include "vmlinux.h" > +#undef bpf_iter_meta > +#undef bpf_iter__task > #include <bpf/bpf_helpers.h> > > char _license[] SEC("license") = "GPL"; > > +struct bpf_iter_meta { > + struct seq_file *seq; > + __u64 session_id; > + __u64 seq_num; > +} __attribute__((preserve_access_index)); > + > +struct bpf_iter__task { > + struct bpf_iter_meta *meta; > + struct task_struct *task; > +} __attribute__((preserve_access_index)); Applied, but I was wondering whether all these structs can be placed in a single header file like bpf_iters.h ? struct bpf_iter_meta is common across all of them. What if next iter patch changes the name in there? We'd need to patch 10 tests? It's unstable api, so it's fine, but considering the churn it seems common header would be good. That .h would include struct bpf_iter__bpf_map, bpf_iter__task, bpf_iter__task_file, etc wdyt?
On Tue, May 19, 2020 at 2:08 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, May 19, 2020 at 12:23:41PM -0700, Andrii Nakryiko wrote: > > b9f4c01f3e0b ("selftest/bpf: Make bpf_iter selftest compilable against old vmlinux.h") > > missed the fact that bpf_iter_test_kern{3,4}.c are not just including > > bpf_iter_test_kern_common.h and need similar bpf_iter_meta re-definition > > explicitly. > > > > Fixes: b9f4c01f3e0b ("selftest/bpf: Make bpf_iter selftest compilable against old vmlinux.h") > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > --- > > .../selftests/bpf/progs/bpf_iter_test_kern3.c | 15 +++++++++++++++ > > .../selftests/bpf/progs/bpf_iter_test_kern4.c | 15 +++++++++++++++ > > 2 files changed, 30 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c > > index 636a00fa074d..13c2c90c835f 100644 > > --- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c > > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c > > @@ -1,10 +1,25 @@ > > // SPDX-License-Identifier: GPL-2.0 > > /* Copyright (c) 2020 Facebook */ > > +#define bpf_iter_meta bpf_iter_meta___not_used > > +#define bpf_iter__task bpf_iter__task___not_used > > #include "vmlinux.h" > > +#undef bpf_iter_meta > > +#undef bpf_iter__task > > #include <bpf/bpf_helpers.h> > > > > char _license[] SEC("license") = "GPL"; > > > > +struct bpf_iter_meta { > > + struct seq_file *seq; > > + __u64 session_id; > > + __u64 seq_num; > > +} __attribute__((preserve_access_index)); > > + > > +struct bpf_iter__task { > > + struct bpf_iter_meta *meta; > > + struct task_struct *task; > > +} __attribute__((preserve_access_index)); > > Applied, but I was wondering whether all these structs can be placed > in a single header file like bpf_iters.h ? > struct bpf_iter_meta is common across all of them. > What if next iter patch changes the name in there? > We'd need to patch 10 tests? It's unstable api, so it's fine, > but considering the churn it seems common header would be good. > That .h would include struct bpf_iter__bpf_map, bpf_iter__task, > bpf_iter__task_file, etc > wdyt? I initially wanted to keep each selftest independent, so that anyone looking for example would just have everything in one file. But I agree, we have quite a bunch of them already, so it makes sense to centralize that in one common header. I'll post a follow-up patch a bit later to consolidate this.
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c index 636a00fa074d..13c2c90c835f 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern3.c @@ -1,10 +1,25 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2020 Facebook */ +#define bpf_iter_meta bpf_iter_meta___not_used +#define bpf_iter__task bpf_iter__task___not_used #include "vmlinux.h" +#undef bpf_iter_meta +#undef bpf_iter__task #include <bpf/bpf_helpers.h> char _license[] SEC("license") = "GPL"; +struct bpf_iter_meta { + struct seq_file *seq; + __u64 session_id; + __u64 seq_num; +} __attribute__((preserve_access_index)); + +struct bpf_iter__task { + struct bpf_iter_meta *meta; + struct task_struct *task; +} __attribute__((preserve_access_index)); + SEC("iter/task") int dump_task(struct bpf_iter__task *ctx) { diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c index b18dc0471d07..0aa71b333cf3 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c @@ -1,10 +1,25 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2020 Facebook */ +#define bpf_iter_meta bpf_iter_meta___not_used +#define bpf_iter__bpf_map bpf_iter__bpf_map___not_used #include "vmlinux.h" +#undef bpf_iter_meta +#undef bpf_iter__bpf_map #include <bpf/bpf_helpers.h> char _license[] SEC("license") = "GPL"; +struct bpf_iter_meta { + struct seq_file *seq; + __u64 session_id; + __u64 seq_num; +} __attribute__((preserve_access_index)); + +struct bpf_iter__bpf_map { + struct bpf_iter_meta *meta; + struct bpf_map *map; +} __attribute__((preserve_access_index)); + __u32 map1_id = 0, map2_id = 0; __u32 map1_accessed = 0, map2_accessed = 0; __u64 map1_seqnum = 0, map2_seqnum1 = 0, map2_seqnum2 = 0;
b9f4c01f3e0b ("selftest/bpf: Make bpf_iter selftest compilable against old vmlinux.h") missed the fact that bpf_iter_test_kern{3,4}.c are not just including bpf_iter_test_kern_common.h and need similar bpf_iter_meta re-definition explicitly. Fixes: b9f4c01f3e0b ("selftest/bpf: Make bpf_iter selftest compilable against old vmlinux.h") Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- .../selftests/bpf/progs/bpf_iter_test_kern3.c | 15 +++++++++++++++ .../selftests/bpf/progs/bpf_iter_test_kern4.c | 15 +++++++++++++++ 2 files changed, 30 insertions(+)