Re: [RFC PATCH v2 3/3] selftests/bpf: exercise task storage from hrtimer_start

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

> selftests/bpf: exercise task storage from hrtimer_start

TL;DR: Test-organization question only: should the new hrtimer task-storage
selftest be a subtest of task_local_storage.c instead of a separate file? The
review finds the test logic correct and the standalone file defensible.

> diff --git a/tools/testing/selftests/bpf/prog_tests/task_storage_hrtimer.c b/tools/testing/selftests/bpf/prog_tests/task_storage_hrtimer.c
> new file mode 100644
> index 000000000000..216e66263bf6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/task_storage_hrtimer.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <sys/timerfd.h>
> +#include <unistd.h>
> +
> +#include <test_progs.h>
> +#include "task_storage_hrtimer.skel.h"
> +
> +#define TRIGGER_COUNT 1000
> +
> +void test_task_storage_hrtimer(void)
> +{

A subsystem pattern flags this as potentially concerning: this adds a new
top-level test file for a narrow variant of behavior that an existing test
in the same directory already covers more generally.

prog_tests/task_local_storage.c already groups the general task-local
storage tests as subtests, including test_nodeadlock(), backed by
progs/task_storage_nodeadlock.c.  That subtest performs the same
bpf_task_storage_get(..., BPF_LOCAL_STORAGE_GET_F_CREATE) followed by
bpf_task_storage_delete() sequence and counts failures, just from a
different hook (lsm.s/socket_post_create).

The only difference here is the attach point, tp_btf/hrtimer_start, which
reaches the task storage allocator under the hrtimer base raw lock on
PREEMPT_RT.

Should this be a new subtest of test_task_local_storage() instead of a
separate prog_tests/task_storage_hrtimer.c?

For balance, the standalone file may well be defensible.  It uses its own
skeleton, as the existing task_local_storage.c subtests also do, so folding
it in would mostly move the test function rather than share setup.  It also
exercises a path that no existing selftest reaches: none of the current
programs that use task storage attach to tracepoints emitted from inside the
hrtimer code, and the programs that do use hrtimer tracepoints
(timer_start_deadlock.c, test_vmlinux.c) do not use task storage.

The test logic itself looks correct: the tp_btf prototype matches
TP_PROTO(hrtimer, mode, was_armed), and timerfd_settime() with a non-zero
it_value emits one trace_hrtimer_start under cpu_base->lock, so the counter
assertions are consistent.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37277547264