On Mon, Feb 03, 2025 at 04:52:52PM -0800, Eduard Zingerman wrote: > > > > --- a/tools/testing/selftests/bpf/progs/arena_atomics.c > > > > +++ b/tools/testing/selftests/bpf/progs/arena_atomics.c > > > [...] > > > > > > > +SEC("raw_tp/sys_enter") > > > > +int load_acquire(const void *ctx) > > > > +{ > > > > + if (pid != (bpf_get_current_pid_tgid() >> 32)) > > > > + return 0; > > > > > > Nit: This check is not needed, since bpf_prog_test_run_opts() is used > > > to run the tests. > > > > Could you explain a bit more why it's not needed? > > > > I read commit 0f4feacc9155 ("selftests/bpf: Adding pid filtering for > > atomics test") which added those 'pid' checks to atomics/ tests. The > > commit message [1] says the purpose was to "make atomics test able to > > run in parallel with other tests", which I couldn't understand. > > > > How using bpf_prog_test_run_opts() makes those 'pid' checks unnecessary? > > > > [1] https://lore.kernel.org/bpf/20211006185619.364369-11-fallentree@xxxxxx/#r > > Hi Peilin, > > The entry point for the test looks as follows: > > void test_arena_atomics(void) > { > ... > skel = arena_atomics__open(); > if (!ASSERT_OK_PTR(skel, "arena atomics skeleton open")) > return; > > if (skel->data->skip_tests) { ... } > err = arena_atomics__load(skel); > if (!ASSERT_OK(err, "arena atomics skeleton load")) > return; > skel->bss->pid = getpid(); > > if (test__start_subtest("add")) > test_add(skel); > ... > > cleanup: > arena_atomics__destroy(skel); > } > > Note arena_atomics__{open,load} calls but absence of the > arena_atomics__attach call. W/o arena_atomics__attach call the > programs would not be hooked to the designated extension points, > e.g. "raw_tp/sys_enter". > > The bpf_prog_test_run_opts() invokes BPF_PROG_TEST_RUN command of the > bpf system call, which does not attach the program either, > but executes jitted code directly with fake context. > (See bpf_prog_ops->test_run callback (method?) and > bpf_prog_test_run_raw_tp()). > > Same happens in prog{,_tests}/arena.c: no attachment happens after > commit [2]. Commit [1] is unnecessary after [2]. > > [2] commit 04fcb5f9a104 ("selftests/bpf: Migrate from bpf_prog_test_run") I see. Thanks for the quick reply as always! Peilin Ye