Re: [RFC PATCH 15/16] selftests/membarrier: Check the memory barriers of each thread

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] selftests/membarrier: run_target() pins CPU before
  registering BPF slot
- [Medium] selftests/membarrier: bpf_setup() fails instead of skipping
  on missing BPF configs
- [Low] selftests/membarrier: reset_shared() reuses BPF counter slots of
  exiting tasks
--

commit 3ab38ee940d58df6774d85bc12aa7c4563dfeb02
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

selftests/membarrier: Check the memory barriers of each thread

This commit adds the membarrier_test_barrier selftest, which uses BPF
kprobes and the sched_switch tracepoint on x86-64 to count per-thread
memory barriers executed during expedited membarrier commands and context
switches.

> diff --git a/tools/testing/selftests/membarrier/membarrier_test_barrier.c b/tools/testing/selftests/membarrier/membarrier_test_barrier.c
> new file mode 100644
> index 0000000000000..46502d2712425
> --- /dev/null
> +++ b/tools/testing/selftests/membarrier/membarrier_test_barrier.c
> @@ -0,0 +1,1253 @@

[ ... ]

> +static const char *bpf_setup(void)
> +{
> +	struct bpf_program *prog;
> +	libbpf_print_fn_t print;
> +	struct bpf_map *map;
> +	const char *err;
> +
> +	setup_pid = getpid();
> +	atexit(bpf_teardown);
> +	if (geteuid())
> +		return "this test needs root";
> +	if (access("/sys/kernel/btf/vmlinux", R_OK))
> +		return "the kernel has no BTF (CONFIG_DEBUG_INFO_BTF)";
> +	kcore_fd = open("/proc/kcore", O_RDONLY | O_CLOEXEC);
> +	if (kcore_fd < 0)
> +		return "cannot read /proc/kcore (CONFIG_PROC_KCORE)";
> +	setup_broken = true;
> +	bpf_obj = bpf_object__open_file(BPF_OBJECT, NULL);
> +	if (!bpf_obj)
> +		return "cannot open " BPF_OBJECT;
> +	if (bpf_object__load(bpf_obj))
> +		return "cannot load the BPF programs";
> +	site_prog = bpf_object__find_program_by_name(bpf_obj, "barrier_site");
> +	prog = bpf_object__find_program_by_name(bpf_obj, "register_thread");
> +	map = bpf_object__find_map_by_name(bpf_obj, "counters");
> +	if (!site_prog || !prog || !map)
> +		return "incomplete BPF object";
> +	register_fd = bpf_program__fd(prog);
> +	counters_fd = bpf_map__fd(map);
> +	map = bpf_object__find_map_by_name(bpf_obj, "site_hits");
> +	hits_fd = map ? bpf_map__fd(map) : -1;
> +	/* The exec test case passes these two to the new program. */
> +	if (fcntl(register_fd, F_SETFD, 0) || fcntl(counters_fd, F_SETFD, 0))
> +		return "fcntl failed";
> +	signal(SIGINT, restore_optprobes);
> +	signal(SIGTERM, restore_optprobes);
> +	signal(SIGHUP, restore_optprobes);
> +	if (!set_optprobes('0'))
> +		return "cannot write " OPTPROBES_FILE;
> +	prog = bpf_object__find_program_by_name(bpf_obj, "sched_switch");
> +	switch_link = prog ? bpf_program__attach(prog) : NULL;
> +	if (!switch_link)
> +		return "cannot attach to the sched_switch tracepoint";

[Severity: Medium]
Does setting setup_broken = true before bpf_object__load() and
bpf_program__attach() cause main() to fail instead of skipping when the
kernel lacks required BPF features?

A kernel with CONFIG_DEBUG_INFO_BTF=y and CONFIG_PROC_KCORE=y can still
lack CONFIG_BPF_EVENTS=y (needed to load kprobe/tracing programs and
BPF_MAP_TYPE_TASK_STORAGE) or have BPF JIT disabled (needed to attach
tp_btf/sched_switch).  When bpf_object__load() or bpf_program__attach()
fails for those reasons, setup_broken is already true, so main() exits via
ksft_exit_fail_msg() rather than ksft_exit_skip().

[ ... ]

> +/* Run the checks of a target. */
> +static void run_target(const struct target *target)
> +{
> +	struct target_stats *stats = &shared->stats[target->idx];
> +	const uint64_t *counters;
> +	long deadline = now_ms() + target->ms;
> +	unsigned long checks = 0;
> +	uint64_t first_ipi, first_sched_in;
> +
> +	if (target->cpu >= 0 && pin_to_cpu(target->cpu))
> +		setup_error("sched_setaffinity");
> +	counters = map_counters();

[Severity: Medium]
Does calling pin_to_cpu(target->cpu) before map_counters() miss the
lazy-TLB context switch in global_expedited_lazy_tlb?

In global_expedited_lazy_tlb, the child pins itself to cpus[1] and then
cpus[2] so that cpus[1]'s idle thread borrows its mm, and then calls
run_target() with target.cpu = cpus[1].  Because pin_to_cpu() migrates the
child back onto cpus[1] before map_counters() runs register_thread to
populate task_slot, the switch from the lazy-TLB idle thread back to the
child on cpus[1] runs while slot_of(next) is still 0:

tools/testing/selftests/membarrier/membarrier_test_barrier.bpf.c:sched_switch() {
    ...
	slot = slot_of(next);
	state->slot = slot;
	state->owed = 0;
	/* The cases which need a barrier after the store to rq->curr. */
	if (slot && next->mm && prev->mm != next->mm) {
		state->owed = OWES_BARRIER;
		count(slot, 1U << MBT_SCHED_IN);
	}
    ...
}

As a result, sched_switch never sets OWES_BARRIER or increments
MBT_SCHED_IN for that lazy-TLB switch.  Calling map_counters() before
pin_to_cpu(target->cpu) registers the thread in task_slot before it
migrates back to cpus[1].

[ ... ]

> +static void reset_shared(void)
> +{
> +	memset(shared, 0, sizeof(*shared));
> +}

[Severity: Low]
Can zeroing shared->next_slot in reset_shared() let an exiting task from a
finished test case clobber the BPF counters of a new thread in the next
test case?

pthread_join() and waitpid() return during do_exit() before the dying task
reaches its final context switch in do_task_dead(), while its task_slot
entry remains attached until free_task().

CPU0                                  CPU1
join_targets() / child_ok()
reset_shared()
  shared->next_slot = 0
start_targets()
  map_counters()
    register_thread() (slot 1)
                                      do_exit()
                                        do_task_dead()
                                          __schedule()
                                            sched_switch() (prev slot 1)
                                              count(1, 1U << MBT_SCHED_OUT)
                                              <-- increments new thread's
                                                  counters

Preserving shared->next_slot across reset_shared() avoids reusing counter
slots across test cases.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113131903.157186-1-mathieu.desnoyers@efficios.com?part=15