On Tue, Mar 1, 2022 at 6:49 PM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> The new Makefile support via SUBSKELS and .skel.h-deps is a mixture
> of LINKED_SKELS and LSKELS. By definition subskeletons require multiple
> BPF object files to be linked together. However, generating the
> subskeleton only requires the library object file and not the final
> program object file.
>
> Signed-off-by: Delyan Kratunov <delyank@xxxxxx>
> ---
> tools/testing/selftests/bpf/Makefile | 18 ++++++++-
> .../selftests/bpf/prog_tests/subskeleton.c | 38 +++++++++++++++++++
> .../bpf/prog_tests/subskeleton_lib.c | 29 ++++++++++++++
> .../selftests/bpf/progs/test_subskeleton.c | 20 ++++++++++
> .../bpf/progs/test_subskeleton_lib.c | 22 +++++++++++
> 5 files changed, 125 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
> create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index fe12b4f5fe20..57da63ba790b 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -326,19 +326,23 @@ endef
> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
>
> LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \
> - linked_vars.skel.h linked_maps.skel.h
> + linked_vars.skel.h linked_maps.skel.h test_subskeleton.skel.h
> +
> +SUBSKELS := test_subskeleton_lib.skel.h
So, unless I'm mistaken, bpf_object__open() will succeed for
"incomplete" BPF object file (e.g., even if they have unresolved
externs, for example). At least that used to be the case.
In such a case, we can totally generate both skeletons and
sub-skeletons for all files for which we currently generate skeletons.
It will keep Makefile simpler and will test sub-skeleton code
generator on a much wider variety of BPF object files. Let's use
.subskel.h naming convention for those. We can even add a simple test
in test_skeleton, test_vmlinux and a bunch of others that "stress
test" skeleton features to make sure that corresponding sub-skeleton
can be opened just fine.
We probably will run into name conflicts for <skel>__open and
<skel>__destroy... So we can either use <skel>__open_subskel and
<skel>__destroy_subskel to disambiguate (might not be a bad idea to
make it clear that we are dealing with "incomplete" sub-skeleton), or
we can just not test skeleton and sub-skeleton in the same user-space
.c file. Not sure if anyone feels strongly about naming, let me know.
>
> 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 \
> map_ptr_kern.c core_kern.c core_kern_overflow.c
> # Generate both light skeleton and libbpf skeleton for these
> LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c
> -SKEL_BLACKLIST += $$(LSKELS)
> +SKEL_BLACKLIST += $$(LSKELS) $$(SUBSKELS)
>
> test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
> linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o
> linked_vars.skel.h-deps := linked_vars1.o linked_vars2.o
> linked_maps.skel.h-deps := linked_maps1.o linked_maps2.o
> +test_subskeleton.skel.h-deps := test_subskeleton_lib.o test_subskeleton.o
> +test_subskeleton_lib.skel.h-deps := test_subskeleton_lib.o
>
> LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps)))
>
> @@ -363,6 +367,7 @@ TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h, \
> $$(filter-out $(SKEL_BLACKLIST) $(LINKED_BPF_SRCS),\
> $$(TRUNNER_BPF_SRCS)))
> TRUNNER_BPF_LSKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.lskel.h, $$(LSKELS) $$(LSKELS_EXTRA))
> +TRUNNER_BPF_SUBSKELS := $$(addprefix $$(TRUNNER_OUTPUT)/,$(SUBSKELS))
> TRUNNER_BPF_SKELS_LINKED := $$(addprefix $$(TRUNNER_OUTPUT)/,$(LINKED_SKELS))
> TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS)
>
> @@ -405,6 +410,14 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
> $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
>
> +$(TRUNNER_BPF_SUBSKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> + $$(call msg,GEN-SUBSKEL,$(TRUNNER_BINARY),$$@)
> + $(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 subskeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> +
> $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
> @@ -441,6 +454,7 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \
> $(TRUNNER_EXTRA_HDRS) \
> $(TRUNNER_BPF_OBJS) \
> $(TRUNNER_BPF_SKELS) \
> + $(TRUNNER_BPF_SUBSKELS) \
> $(TRUNNER_BPF_LSKELS) \
> $(TRUNNER_BPF_SKELS_LINKED) \
> $$(BPFOBJ) | $(TRUNNER_OUTPUT)
> diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton.c b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> new file mode 100644
> index 000000000000..651aafc28e7f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +
year is off
> +#include <test_progs.h>
> +#include "test_subskeleton.skel.h"
> +
> +extern void subskeleton_lib_setup(struct bpf_object *obj);
> +extern int subskeleton_lib_subresult(struct bpf_object *obj);
> +
> +void test_subskeleton(void)
> +{
> + int duration = 0, err, result;
> + struct test_subskeleton *skel;
> +
> + skel = test_subskeleton__open();
> + if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
no CHECK()s
> + return;
> +
> + skel->rodata->rovar1 = 10;
> +
> + err = test_subskeleton__load(skel);
> + if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
CHECK
> + goto cleanup;
> +
> + subskeleton_lib_setup(skel->obj);
> +
> + err = test_subskeleton__attach(skel);
> + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
CHECK
> + goto cleanup;
> +
> + /* trigger tracepoint */
> + usleep(1);
> +
> + result = subskeleton_lib_subresult(skel->obj) * 10;
> + ASSERT_EQ(skel->bss->out1, result, "unexpected calculation");
> +cleanup:
> + test_subskeleton__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c b/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
> new file mode 100644
> index 000000000000..f7f98b3febaf
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton_lib.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +
outdated year?
> +#include <test_progs.h>
> +#include <bpf/libbpf.h>
> +
> +#include "test_subskeleton_lib.skel.h"
> +
> +void subskeleton_lib_setup(struct bpf_object *obj)
> +{
> + struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +
> + ASSERT_OK_PTR(lib, "open subskeleton");
return on failed assert, otherwise SIGSEGV
> +
> + *lib->data.var1 = 1;
> + *lib->bss.var2 = 2;
> + lib->bss.var3->var3_1 = 3;
> + lib->bss.var3->var3_2 = 4;
> +}
> +
> +int subskeleton_lib_subresult(struct bpf_object *obj)
> +{
> + struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +
> + ASSERT_OK_PTR(lib, "open subskeleton");
> +
> + ASSERT_EQ(*lib->bss.libout1, 1 + 2 + 3 + 4, "lib subresult");
> + return *lib->bss.libout1;
> +}
I'm not sure we really need to have a separate user-space file to
simulate a library code. Let's have this library setup code in the
selftest file itself
> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton.c b/tools/testing/selftests/bpf/progs/test_subskeleton.c
> new file mode 100644
> index 000000000000..bad3970718cb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +const int rovar1;
> +int out1;
see below, let's have some shared stuff between skeleton and
subskeleton (.kconfig, variable used from lib, variable defined in
lib, etc). Think creatively on how you could break codegen :)
As we want to add maps, I'd also use extern maps for more coverage
> +
> +extern int lib_routine(void);
> +
> +SEC("raw_tp/sys_enter")
> +int handler1(const void *ctx)
> +{
> + out1 = lib_routine() * rovar1;
> + return 0;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> +int VERSION SEC("version") = 1;
see below, no VERSION nowadays
> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
> new file mode 100644
> index 000000000000..23c7f24997a7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +int var1 = -1;
> +int var2;
> +struct {
> + int var3_1;
> + __s64 var3_2;
> +} var3;
> +int libout1;
we should also test:
- .kconfig externs
- __weak variables
- .rodata variable (like Alexei already mentioned)
- let's also have an array variable (C uses non-uniform syntax for
pointer to an array)
- extern .data variable defined in another file
> +
> +int lib_routine(void)
> +{
> + libout1 = var1 + var2 + var3.var3_1 + var3.var3_2;
nit: extra space after =
> + return libout1;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> +int VERSION SEC("version") = 1;
VERSION is obsolete, please drop
> --
> 2.34.1
