Re: [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton

On Wed, Oct 20, 2021 at 3:37 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

On Thu, Oct 21, 2021 at 03:47:19AM IST, Andrii Nakryiko wrote:

quoted

On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
Include both light and libbpf skeleton in same file to test both of them
together.

In c48e51c8b07a ("bpf: selftests: Add selftests for module kfunc support"),
I added support for generating both lskel and libbpf skel for a BPF
object, however the name parameter for bpftool caused collisions when
included in same file together. This meant that every test needed a
separate file for a libbpf/light skeleton separation instead of
subtests.

Change that by appending a "_light" suffix to the name for files listed
in LSKELS_EXTRA, such that both light and libbpf skeleton can be used in
the same file for subtests, leading to better code sharing.

While at it, improve the build output by saying GEN-LSKEL instead of
GEN-SKEL for light skeleton generation recipe.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/Makefile          |  7 ++--
 .../selftests/bpf/prog_tests/ksyms_btf.c      | 35 +++++++++++++++-
 .../selftests/bpf/prog_tests/ksyms_module.c   | 40 +++++++++++++++++--
 .../bpf/prog_tests/ksyms_module_libbpf.c      | 28 -------------
 .../selftests/bpf/progs/test_ksyms_weak.c     |  3 +-
 5 files changed, 74 insertions(+), 39 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 498222543c37..1c3c8befc249 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile

@@ -325,7 +325,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

@@ -399,12 +399,13 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
        $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@

 $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
-       $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
+       $$(call msg,GEN-LSKEL,$(TRUNNER_BINARY),$$@)

This breaks nice output alignment:

  GEN-SKEL [test_progs-no_alu32] bpf_iter_tcp4.skel.h
  GEN-LSKEL [test_progs-no_alu32] trace_vprintk.lskel.h

Ok, I'll drop it.

quoted

Isn't ".lskel.h" suffix enough to distinguish them?

quoted

        $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
        $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
        $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
        $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
-       $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
+       $$(eval LSKEL_NAME := $$(notdir $$(<:.o=$$(if $$(filter $$(notdir $$(<:.o=.c)),$(LSKELS_EXTRA)),_light,))))

eval inside eval?.. Wow, do we really need that? If you just want to

I knew you'd like it ;-)

yeah, big fan, obviously :)

quoted

add _light (I suggest _lskel though, it will make for a more
meaningful and recognizable names in user-space code) suffix, do it
for all light skeletons unconditionally and keep it simple?

I avoided this because it would be a lot of unecessary changes, while we only
need this for two files. Every struct/function name will get the suffix, but I
can do it if the Makefile stuff looks too horrible.

There are 10 light skeleton-using selftests, it's not a problem to do
one-time renaming. Currently there are only two files that do both
skel and lskel, but that number will inevitably grow, so better to set
everything up now.

quoted

quoted

+       $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(LSKEL_NAME) > $$@

 $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
        $$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.o))

[...]

--
Kartikeya