On Wed, Mar 30, 2022 at 12:51 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote: > > According to the discussions in [1] everything from the samples/bpf > directory should be moved out or deleted. > > Move the spintest program from BPF samples to BPF selftests. > There are no functional changes. BPF skeleton has been used for loading, > etc. > Great that you are helping with selftests consolidation, thanks! I have few comments which will require a new revision, but overall this is moving in the right direction. > [1] https://lore.kernel.org/all/CAEf4BzYv3ONhy-JnQUtknzgBSK0gpm9GBJYtbAiJQe50_eX7Uw@xxxxxxxxxxxxxx/ > > Signed-off-by: Puranjay Mohan <puranjay12@xxxxxxxxx> > --- > samples/bpf/Makefile | 3 -- > .../selftests/bpf/prog_tests/spintest.c | 43 +++++++------------ > .../testing/selftests/bpf/progs/test_spin.c | 15 +++---- > 3 files changed, 23 insertions(+), 38 deletions(-) > rename samples/bpf/spintest_user.c => tools/testing/selftests/bpf/prog_tests/spintest.c (63%) > rename samples/bpf/spintest_kern.c => tools/testing/selftests/bpf/progs/test_spin.c (86%) > [...] > > - if (load_kallsyms()) { > - printf("failed to process /proc/kallsyms\n"); > - return 2; > - } > + err = load_kallsyms(); do we need to use kallsyms? selftests generally follows the latest kernel, so whatever attach target doesn't exist in the kernel we should just remove > + if (!ASSERT_OK(err, "load_kallsyms")) > + return; > + skel = test_spin__open_and_load(); > > - snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > - obj = bpf_object__open_file(filename, NULL); > - if (libbpf_get_error(obj)) { > - fprintf(stderr, "ERROR: opening BPF object file failed\n"); > - obj = NULL; > - goto cleanup; > - } > + if (!ASSERT_OK_PTR(skel, "test_spin__open_and_load")) > + return; > > - /* load BPF program */ > - if (bpf_object__load(obj)) { > - fprintf(stderr, "ERROR: loading BPF object file failed\n"); > - goto cleanup; > - } > - > - map_fd = bpf_object__find_map_fd_by_name(obj, "my_map"); > - if (map_fd < 0) { > - fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > - goto cleanup; > - } > + map_fd = bpf_map__fd(skel->maps.my_map); > > - bpf_object__for_each_program(prog, obj) { > + bpf_object__for_each_program(prog, skel->obj) { > section = bpf_program__section_name(prog); > if (sscanf(section, "kprobe/%s", symbol) != 1) > continue; > @@ -52,7 +41,8 @@ int main(int ac, char **argv) > /* Attach prog only when symbol exists */ > if (ksym_get_addr(symbol)) { > links[j] = bpf_program__attach(prog); > - if (libbpf_get_error(links[j])) { > + err = libbpf_get_error(links[j]); > + if (!ASSERT_OK(err, "bpf_program__attach")) { > fprintf(stderr, "bpf_program__attach failed\n"); > links[j] = NULL; > goto cleanup; there are a bunch of leftover prints, let's get rid of them and/or turn them into ASSERT_xxx() there is sleep(1) in each of 5 iterations, this slows down the selftests a lot. Let's get rid of it. there is also assert(), there shouldn't be any calls to assert() crashing the process in selftests > @@ -89,5 +79,4 @@ int main(int ac, char **argv) > bpf_link__destroy(links[j]); let's use skeleton's links "storage" and simplify the cleanup significantly. > > bpf_object__close(obj); this should be test_spin__destroy() instead > - return 0; > } > diff --git a/samples/bpf/spintest_kern.c b/tools/testing/selftests/bpf/progs/test_spin.c > similarity index 86% > rename from samples/bpf/spintest_kern.c > rename to tools/testing/selftests/bpf/progs/test_spin.c > index 455da77319d9..531783fe6cb9 100644 > --- a/samples/bpf/spintest_kern.c > +++ b/tools/testing/selftests/bpf/progs/test_spin.c > @@ -4,14 +4,14 @@ > * modify it under the terms of version 2 of the GNU General Public > * License as published by the Free Software Foundation. > */ > -#include <linux/skbuff.h> > -#include <linux/netdevice.h> > -#include <linux/version.h> > -#include <uapi/linux/bpf.h> > -#include <uapi/linux/perf_event.h> > +#include <vmlinux.h> > #include <bpf/bpf_helpers.h> > #include <bpf/bpf_tracing.h> > > +#ifndef PERF_MAX_STACK_DEPTH > +#define PERF_MAX_STACK_DEPTH 127 > +#endif > + > struct { > __uint(type, BPF_MAP_TYPE_HASH); > __type(key, long); > @@ -27,8 +27,8 @@ struct { > > struct { > __uint(type, BPF_MAP_TYPE_STACK_TRACE); > - __uint(key_size, sizeof(u32)); > - __uint(value_size, PERF_MAX_STACK_DEPTH * sizeof(u64)); > + __uint(key_size, sizeof(__u32)); > + __uint(value_size, PERF_MAX_STACK_DEPTH * sizeof(__u64)); > __uint(max_entries, 10000); > } stackmap SEC(".maps"); > > @@ -66,4 +66,3 @@ SEC("kprobe/__htab_percpu_map_update_elem")PROG(p16) > SEC("kprobe/htab_map_alloc")PROG(p17) I don't see why we need this PROG(pXX) magic instead of just doing a function call. Let's have a common function called from each of the defined kprobe program. > > char _license[] SEC("license") = "GPL"; > -u32 _version SEC("version") = LINUX_VERSION_CODE; > -- > 2.35.1 >
