| Message ID | 20211006002853.308945-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Typeless/weak ksym for gen_loader + misc fixups | expand |
On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet. > Create a file for testing libbpf skeleton as well, so that both > gen_loader and libbpf get tested. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> [...] > diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c > new file mode 100644 > index 000000000000..b75725e28647 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c > @@ -0,0 +1,31 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <test_progs.h> > +#include "test_ksyms_weak.skel.h" > + > +void test_ksyms_weak_libbpf(void) This is (almost?) the same as test_weak_syms(), right? Why do we need both? > +{ > + struct test_ksyms_weak *skel; > + struct test_ksyms_weak__data *data; > + int err; > + > + skel = test_ksyms_weak__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load")) > + return; [...] > diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c > index 5f8379aadb29..521e7b99db08 100644 > --- a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c > +++ b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c > @@ -21,7 +21,6 @@ __u64 out__non_existent_typed = -1; > extern const struct rq runqueues __ksym __weak; /* typed */ > extern const void bpf_prog_active __ksym __weak; /* typeless */ > > - > /* non-existent weak symbols. */ > > /* typeless symbols, default to zero. */ > @@ -38,7 +37,7 @@ int pass_handler(const void *ctx) > /* tests existing symbols. */ > rq = (struct rq *)bpf_per_cpu_ptr(&runqueues, 0); > if (rq) > - out__existing_typed = rq->cpu; > + out__existing_typed = 0; Why do we need this change? > out__existing_typeless = (__u64)&bpf_prog_active; > > /* tests non-existent symbols. */ > -- > 2.33.0 >
On Fri, Oct 08, 2021 at 02:03:49AM IST, Song Liu wrote: > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet. > > Create a file for testing libbpf skeleton as well, so that both > > gen_loader and libbpf get tested. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > [...] > > diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c > > new file mode 100644 > > index 000000000000..b75725e28647 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c > > @@ -0,0 +1,31 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include <test_progs.h> > > +#include "test_ksyms_weak.skel.h" > > + > > +void test_ksyms_weak_libbpf(void) > > This is (almost?) the same as test_weak_syms(), right? Why do we need both? > One includes lskel.h (light skeleton), the other includes skel.h (libbpf skeleton). Trying to include both in the same file, it ends up redefining the same struct. I am not sure whether adding a prefix/suffix to light skeleton struct names is possible now, maybe through another option to bpftool in addition to name? > > +{ > > + struct test_ksyms_weak *skel; > > + struct test_ksyms_weak__data *data; > > + int err; > > + > > + skel = test_ksyms_weak__open_and_load(); > > + if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load")) > > + return; > > [...] > > > diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c > > index 5f8379aadb29..521e7b99db08 100644 > > --- a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c > > +++ b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c > > @@ -21,7 +21,6 @@ __u64 out__non_existent_typed = -1; > > extern const struct rq runqueues __ksym __weak; /* typed */ > > extern const void bpf_prog_active __ksym __weak; /* typeless */ > > > > - > > /* non-existent weak symbols. */ > > > > /* typeless symbols, default to zero. */ > > @@ -38,7 +37,7 @@ int pass_handler(const void *ctx) > > /* tests existing symbols. */ > > rq = (struct rq *)bpf_per_cpu_ptr(&runqueues, 0); > > if (rq) > > - out__existing_typed = rq->cpu; > > + out__existing_typed = 0; > > Why do we need this change? > Since they share the same BPF object for generating skeleton, it needs to remove dependency on CO-RE which gen_loader does not support. If it is kept, we get this: ... libbpf: // TODO core_relo: prog 0 insn[5] rq kind 0 libbpf: prog 'pass_handler': relo #0: failed to relocate: -95 libbpf: failed to perform CO-RE relocations: -95 libbpf: failed to load object 'test_ksyms_weak' ... > > out__existing_typeless = (__u64)&bpf_prog_active; > > > > /* tests non-existent symbols. */ > > -- > > 2.33.0 > > -- Kartikeya
On Fri, Oct 08, 2021 at 02:16:09AM IST, Kumar Kartikeya Dwivedi wrote: > On Fri, Oct 08, 2021 at 02:03:49AM IST, Song Liu wrote: > > On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet. > > > Create a file for testing libbpf skeleton as well, so that both > > > gen_loader and libbpf get tested. > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > [...] > > > diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c > > > new file mode 100644 > > > index 000000000000..b75725e28647 > > > --- /dev/null > > > +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c > > > @@ -0,0 +1,31 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +#include <test_progs.h> > > > +#include "test_ksyms_weak.skel.h" > > > + > > > +void test_ksyms_weak_libbpf(void) > > > > This is (almost?) the same as test_weak_syms(), right? Why do we need both? > > > > One includes lskel.h (light skeleton), the other includes skel.h (libbpf > skeleton). Trying to include both in the same file, it ends up redefining the > same struct. I am not sure whether adding a prefix/suffix to light skeleton > struct names is possible now, maybe through another option to bpftool in > addition to name? Sorry, I misremembered. The name option is enough, it is because of how I did it in the Makefile (using LSKELS_EXTRA). I'll fix this in the next spin. > [...] -- Kartikeya
On Thu, Oct 7, 2021 at 1:46 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > [...] > > > > > > /* typeless symbols, default to zero. */ > > > @@ -38,7 +37,7 @@ int pass_handler(const void *ctx) > > > /* tests existing symbols. */ > > > rq = (struct rq *)bpf_per_cpu_ptr(&runqueues, 0); > > > if (rq) > > > - out__existing_typed = rq->cpu; > > > + out__existing_typed = 0; > > > > Why do we need this change? > > > > Since they share the same BPF object for generating skeleton, it needs to remove > dependency on CO-RE which gen_loader does not support. > > If it is kept, we get this: > ... > libbpf: // TODO core_relo: prog 0 insn[5] rq kind 0 > libbpf: prog 'pass_handler': relo #0: failed to relocate: -95 > libbpf: failed to perform CO-RE relocations: -95 > libbpf: failed to load object 'test_ksyms_weak' I see. Thanks for the explanation. Song
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index c5c9a9f50d8d..4ae5bc852ca8 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -317,7 +317,7 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \ LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \ test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c # Generate both light skeleton and libbpf skeleton for these -LSKELS_EXTRA := test_ksyms_module.c +LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c SKEL_BLACKLIST += $$(LSKELS) test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c index cf3acfa5a91d..21ff02d47076 100644 --- a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c @@ -6,7 +6,7 @@ #include <bpf/btf.h> #include "test_ksyms_btf.skel.h" #include "test_ksyms_btf_null_check.skel.h" -#include "test_ksyms_weak.skel.h" +#include "test_ksyms_weak.lskel.h" static int duration; @@ -89,11 +89,11 @@ static void test_weak_syms(void) int err; skel = test_ksyms_weak__open_and_load(); - if (CHECK(!skel, "test_ksyms_weak__open_and_load", "failed\n")) + if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load")) return; err = test_ksyms_weak__attach(skel); - if (CHECK(err, "test_ksyms_weak__attach", "skeleton attach failed: %d\n", err)) + if (!ASSERT_OK(err, "test_ksyms_weak__attach")) goto cleanup; /* trigger tracepoint */ diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c new file mode 100644 index 000000000000..b75725e28647 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <test_progs.h> +#include "test_ksyms_weak.skel.h" + +void test_ksyms_weak_libbpf(void) +{ + struct test_ksyms_weak *skel; + struct test_ksyms_weak__data *data; + int err; + + skel = test_ksyms_weak__open_and_load(); + if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load")) + return; + + err = test_ksyms_weak__attach(skel); + if (!ASSERT_OK(err, "test_ksyms_weak__attach")) + goto cleanup; + + /* trigger tracepoint */ + usleep(1); + + data = skel->data; + ASSERT_EQ(data->out__existing_typed, 0, "existing typed ksym"); + ASSERT_NEQ(data->out__existing_typeless, -1, "existing typeless ksym"); + ASSERT_EQ(data->out__non_existent_typeless, 0, "nonexistent typeless ksym"); + ASSERT_EQ(data->out__non_existent_typed, 0, "nonexistent typed ksym"); + +cleanup: + test_ksyms_weak__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c index 5f8379aadb29..521e7b99db08 100644 --- a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c +++ b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c @@ -21,7 +21,6 @@ __u64 out__non_existent_typed = -1; extern const struct rq runqueues __ksym __weak; /* typed */ extern const void bpf_prog_active __ksym __weak; /* typeless */ - /* non-existent weak symbols. */ /* typeless symbols, default to zero. */ @@ -38,7 +37,7 @@ int pass_handler(const void *ctx) /* tests existing symbols. */ rq = (struct rq *)bpf_per_cpu_ptr(&runqueues, 0); if (rq) - out__existing_typed = rq->cpu; + out__existing_typed = 0; out__existing_typeless = (__u64)&bpf_prog_active; /* tests non-existent symbols. */
Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet. Create a file for testing libbpf skeleton as well, so that both gen_loader and libbpf get tested. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/testing/selftests/bpf/Makefile | 2 +- .../selftests/bpf/prog_tests/ksyms_btf.c | 6 ++-- .../bpf/prog_tests/ksyms_weak_libbpf.c | 31 +++++++++++++++++++ .../selftests/bpf/progs/test_ksyms_weak.c | 3 +- 4 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c