Re: [PATCH bpf 2/2] selftests/bpf: Add a selftest for program allocation/freeing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/bpf: standalone stress binary bypasses test_progs and kselftest
- [Medium] selftests/bpf: hardcoded 64-CPU check and contiguous CPU IDs in pin_cpu()
- [Medium] selftests/bpf: 64 KiB stack in run_threads() below PTHREAD_STACK_MIN
- [Low] selftests/bpf: commit message omits deadlock details and test design
--

commit f26b450586e3e82488cb0fdfca492b1cd1920f9b
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: Add a selftest for program allocation/freeing

This commit adds a standalone stress test under tools/testing/selftests/bpf/
that repeatedly loads 65,536 socket filter programs across 64 CPUs, closes
them from CPU 0 under SCHED_FIFO, and spawns 1,024 waiter threads.

>     selftests/bpf: Add a selftest for program allocation/freeing
>     
>     Add a selftest that stresses BPF program allocation/free
>     paths.
>     
>     Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>

[Severity: Low]
This isn't a bug, but the commit message only states that a selftest is
added without explaining the underlying kernel bug mechanism or why the
workload in run_burst() is structured this way.

Could the commit message be expanded to describe the deadlock scenario being
tested (such as deferred __bpf_prog_put_noref() work items saturating CPU 0's
system_percpu_wq max_active limit while blocked on pack_mutex and starving
the vmalloc purge_vmap_node work item), the rationale for the multi-phase
workload parameters in run_burst(), and why the binary is placed in
TEST_GEN_PROGS_EXTENDED?

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 93c707116fad0..e59e07c4bd121 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -137,6 +137,7 @@ TEST_KMOD_TARGETS = $(addprefix $(OUTPUT)/,$(TEST_KMODS))
>  
>  # Compile but not part of 'make run_tests'
>  TEST_GEN_PROGS_EXTENDED = \
> +	bpf_prog_free_stress \
>  	bench \
>  	flow_dissector_load \
>  	test_cpp \

[Severity: Medium]
By adding bpf_prog_free_stress only to TEST_GEN_PROGS_EXTENDED without a
corresponding runner in TEST_PROGS or prog_tests/, neither make run_tests
nor BPF CI (test_progs) will ever execute this test.

In addition, tools/testing/selftests/bpf/bpf_prog_free_stress.c defines a
standalone main() that reports status via printf() and err()/errx(), and
reimplements program loading in load_prog() via raw syscall(__NR_bpf,
BPF_PROG_LOAD, ...) instead of using libbpf's bpf_prog_load() and either
test_progs ASSERT_*() macros (like existing stress tests in prog_tests/ such
as task_work_stress.c, fexit_stress.c, and bpf_mod_race.c) or kselftest.h
ksft_* reporting helpers.

Should this test be integrated as a test_progs subtest under prog_tests/ (or
use libbpf's bpf_prog_load() and kselftest.h reporting if kept standalone)?

> diff --git a/tools/testing/selftests/bpf/bpf_prog_free_stress.c b/tools/testing/selftests/bpf/bpf_prog_free_stress.c
> new file mode 100644
> index 0000000000000..8c761845c7032
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/bpf_prog_free_stress.c
> @@ -0,0 +1,184 @@

[ ... ]

> +#define LOAD_CPUS 64
> +#define PROGRAMS 65536
> +#define WAITERS 1024
> +#define DEFAULT_DURATION_SEC 5
> +
> +static int fds[PROGRAMS];
> +static const struct bpf_insn insns[] = {
> +	BPF_MOV64_IMM(BPF_REG_0, 0),
> +	BPF_EXIT_INSN(),
> +};
> +
> +static void pin_cpu(int cpu)
> +{
> +	cpu_set_t mask;
> +
> +	CPU_ZERO(&mask);
> +	CPU_SET(cpu, &mask);
> +	if (sched_setaffinity(0, sizeof(mask), &mask))
> +		err(EXIT_FAILURE, "sched_setaffinity CPU%d", cpu);
> +}

[ ... ]

> +static void *load_worker(void *arg)
> +{
> +	int cpu = (uintptr_t)arg;
> +	int first = cpu * (PROGRAMS / LOAD_CPUS);
> +	int i;
> +
> +	pin_cpu(cpu);
> +	for (i = first; i < first + PROGRAMS / LOAD_CPUS; i++)
> +		fds[i] = load_prog();
> +	return NULL;
> +}

[ ... ]

> +static void run_threads(int count, void *(*worker)(void *))
> +{
> +	pthread_t threads[WAITERS];
> +	pthread_attr_t attr;
> +	int i;
> +
> +	check_pthread(pthread_attr_init(&attr), "pthread_attr_init");
> +	check_pthread(pthread_attr_setstacksize(&attr, 64 * 1024),
> +		      "pthread_attr_setstacksize");

[Severity: Medium]
In run_threads(), does setting a hardcoded 64 KiB stack size via
pthread_attr_setstacksize(&attr, 64 * 1024) fail with EINVAL on architectures
where PTHREAD_STACK_MIN is larger than 64 KiB?

For example, on powerpc64le (where PTHREAD_STACK_MIN is 131072) or aarch64
with 64 KiB pages, pthread_attr_setstacksize() returns EINVAL when the
requested size is smaller than PTHREAD_STACK_MIN, causing check_pthread() to
abort the test with EXIT_FAILURE on the first burst.

Could this clamp the stack size to at least PTHREAD_STACK_MIN?

[ ... ]

> +int main(int argc, char **argv)
> +{
> +	struct rlimit limit = { PROGRAMS + 1024, PROGRAMS + 1024 };
> +	long duration = parse_duration(argc, argv);
> +	long start;
> +	int round = 0;
> +
> +	setvbuf(stdout, NULL, _IOLBF, 0);
> +	if (sysconf(_SC_NPROCESSORS_ONLN) < LOAD_CPUS)
> +		errx(EXIT_FAILURE, "need at least %d online CPUs", LOAD_CPUS);

[Severity: Medium]
What happens in main() and load_worker() when this test runs on systems with
fewer than 64 online CPUs, or on systems with at least 64 online CPUs where
CPU IDs 0 through 63 are not all online and permitted by the process's CPU
affinity mask?

First, main() exits with EXIT_FAILURE when sysconf(_SC_NPROCESSORS_ONLN) <
LOAD_CPUS (64), or when unprivileged calls to setrlimit(),
sched_setscheduler(), or BPF_PROG_LOAD fail. Per kselftest policy, missing
environment or hardware prerequisites should skip rather than fail. Moreover,
since __purge_vmap_area_lazy() in mm/vmalloc.c queues asynchronous purge work
onto system_percpu_wq whenever nr_purge_nodes >= 2, could the test scale to
the number of available CPUs in the affinity mask and only skip when fewer
than 2 CPUs are available?

Second, load_worker() passes the worker index 0..LOAD_CPUS-1 directly to
pin_cpu(cpu), while main() and load_waiter() hardcode pin_cpu(0). Because
sysconf(_SC_NPROCESSORS_ONLN) only checks the total count of online CPUs,
sched_setaffinity() in pin_cpu() will fail with EINVAL if any CPU in 0..63 is
offline or excluded by the current cpuset or taskset affinity mask. Could
main() query sched_getaffinity() at startup and map workers onto the set of
usable CPU IDs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102073439.6822-1-emil@etsalapatis.com?part=2