Re: [PATCH bpf-next v1 4/6] bpf: selftests: Move test_ksyms_weak test to lskel, add libbpf test

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 [off-list ref] wrote:

quoted

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>

[...]

quoted

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?

quoted

+{
+       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;

[...]

quoted

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'
...

quoted

        out__existing_typeless = (__u64)&bpf_prog_active;

        /* tests non-existent symbols. */
--
2.33.0

--
Kartikeya