Re: [PATCH bpf-next v6 4/4] selftests/bpf: Add task work BUSY window test

[luyun <luyun_611@163.com>]

在 2026/10/10 13:20, bot+bpf-ci@kernel.org 写道:
>> selftests/bpf: Add task work BUSY window test
> TL;DR: Test issues only, no kernel bug: the /sys/kernel/realtime PREEMPT_RT
> check looks dead (kconfig could skip earlier); helpers duplicate the race
> test; unused counters; redundant fprintf()s.

Thanks for the review! All the points look reasonable.

For v7 I'll:
- use the CONFIG_PREEMPT_RT kconfig extern via skel->kconfig to skip
   before attaching, instead of the /sys/kernel/realtime probe;
- share the status accessors, polling helper/bounds and the value
   layout with the cancellation race test;
- drop the unused ACK_SEEN/TIMEOUT counters (and the stats map with
   them) and the redundant fprintf()s.

I will send v7 shortly.

---

Thanks

Yun Lu


>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_task_work.c b/tools/testing/selftests/bpf/prog_tests/test_task_work.c
>> index f9fd9adddfe6..eb0fe51617c5 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/test_task_work.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_task_work.c
> [ ... ]
>
>> @@ -436,3 +438,563 @@ void serial_test_task_work_race(void)
> [ ... ]
>
>> +/*
>> + * Positive confirmation of PREEMPT_RT only: not every RT kernel provides
>> + * this file, and its absence proves nothing.  The definitive check for
>> + * such kernels is the irq_work thread observation inside the BPF program.
>> + */
>> +static bool task_work_busy_rt_confirmed(void)
>> +{
>> +	char buf[2] = {};
>> +	int fd, n;
>> +
>> +	fd = open("/sys/kernel/realtime", O_RDONLY | O_CLOEXEC);
>> +	if (fd < 0)
>> +		return false;
>> +	n = read(fd, buf, sizeof(buf) - 1);
>> +	close(fd);
>> +	return n > 0 && buf[0] == '1';
>> +}
> A subsystem pattern flags this as potentially concerning: the selftests
> guidance is to reuse existing shared test helpers instead of hand-rolling
> file I/O for something that is already provided.
>
> Does any in-tree code provide /sys/kernel/realtime?  Nothing under kernel/
> or init/ in this tree defines it, so task_work_busy_rt_confirmed() would seem
> to always return false, and the early skip in serial_test_task_work_busy()
> would never fire.  The comment above the
> function says "not every RT kernel provides this file", and the commit
> message says the flag is used "when present".
>
> On a PREEMPT_RT kernel the test would then only skip after
> task_work_busy_wait_window() has polled for TASK_WORK_BUSY_POLL_ITERS *
> TASK_WORK_BUSY_POLL_US (more than 2s) and task_work_busy_run_attempt() has
> returned TWB_RT.  The end result is still a skip, so this looks like a dead
> check plus a delay rather than a wrong result.
>
> bpf_experimental.h already declares:
>
>      extern bool CONFIG_PREEMPT_RT __kconfig __weak;
>
> and other prog_tests read kconfig externs through the skeleton, for example
> stacktrace_ips.c:
>
>      if (!skel->kconfig->CONFIG_UNWINDER_ORC) {
>
> Would a kconfig extern in progs/task_work_busy.c, checked through
> skel->kconfig after task_work_busy__open_and_load(), let the test skip up
> front instead?
>
>> +static int task_work_busy_status(struct task_work_busy *skel, int idx,
>> +				 __s64 *value)
>> +{
>> +	return bpf_map_lookup_elem(bpf_map__fd(skel->maps.status), &idx,
>> +				   value);
>> +}
>> +
>> +static int task_work_busy_set_status(struct task_work_busy *skel, int idx,
>> +				     __s64 value)
>> +{
>> +	return bpf_map_update_elem(bpf_map__fd(skel->maps.status), &idx,
>> +				   &value, BPF_ANY);
>> +}
> this isn't a bug, but task_work_busy_status() and
> task_work_busy_set_status() look identical to task_work_race_status() and
> task_work_race_set_status() earlier in this file, apart from the skeleton
> type.
>
> The same applies to other pieces: struct task_work_busy_value has the same
> layout as struct task_work_race_value, TASK_WORK_BUSY_POLL_US,
> TASK_WORK_BUSY_POLL_ITERS and TASK_WORK_BUSY_START_POLL_ITERS have the same
> values (100/20000/50000) as the TASK_WORK_RACE_* macros, and
> task_work_busy_wait_seq() repeats the polling logic of
> task_work_race_check_done().
>
> Both skeletons expose a status map with the same key and value types.  Could
> both tests share helpers that take the status map (or its fd) and a shared
> set of poll constants instead?
>
> [ ... ]
>
>> +	if (!ASSERT_OK(task_work_busy_stat(skel, BUSY_STAT_WINDOW, &stat_window),
>> +		       "window stat") ||
>> +	    !ASSERT_OK(task_work_busy_stat(skel, BUSY_STAT_TOTAL, &stat_total),
>> +		       "total stat") ||
>> +	    !ASSERT_OK(task_work_busy_stat(skel, BUSY_STAT_FILTERED, &stat_filtered),
>> +		       "filtered stat") ||
>> +	    !ASSERT_OK(task_work_busy_stat(skel, BUSY_STAT_G1_RETRY, &stat_g1r),
>> +		       "gen1 retry stat") ||
>> +	    !ASSERT_OK(task_work_busy_stat(skel, BUSY_STAT_G2_RETRY, &stat_g2r),
>> +		       "gen2 retry stat"))
>> +		goto cleanup;
> this isn't a bug, but are BUSY_STAT_ACK_SEEN and BUSY_STAT_TIMEOUT meant to
> be read here?
>
> The fexit program in progs/task_work_busy.c bumps both counters for every
> closed window, and enum task_work_busy_stat mirrors them, but this function
> reads and prints only WINDOW, TOTAL, FILTERED, G1_RETRY and G2_RETRY.  A
> grep of tools/testing/selftests/bpf finds no userspace reader for ACK_SEEN
> or TIMEOUT.
>
> Could ACK_SEEN be checked against valid and TIMEOUT against missed (or both
> added to the summary line below), or can they be dropped from both enums and
> the BPF program?
>
>> +	fprintf(stderr,
>> +		"task_work_busy: %d/%d valid, %d missed, %d attempts, max_wait %lld ns, windows %llu, adds %llu, filtered %llu, g1_retries %llu, g2_retries %llu\n",
>> +		valid, TASK_WORK_BUSY_ROUNDS, missed, attempts_started,
>> +		(long long)max_wait_ns, (unsigned long long)stat_window,
>> +		(unsigned long long)stat_total,
>> +		(unsigned long long)stat_filtered,
>> +		(unsigned long long)stat_g1r, (unsigned long long)stat_g2r);
> [ ... ]
>
>> +	if (!hard_fail) {
>> +		if (!ASSERT_EQ(stat_window, (__u64)(valid + missed),
>> +			       "window accounting"))
>> +			fprintf(stderr,
>> +				"windows entered %llu, valid %d, missed %d\n",
>> +				(unsigned long long)stat_window, valid, missed);
>> +		if (!ASSERT_GE(stat_total, (__u64)(2 * valid),
>> +			       "task_work_add calls seen"))
>> +			fprintf(stderr,
>> +				"only %llu task_work_add calls for %d valid rounds\n",
>> +				(unsigned long long)stat_total, valid);
>> +		if (valid != TASK_WORK_BUSY_ROUNDS)
>> +			fprintf(stderr,
>> +				"busy coverage: %d/%d valid, %d missed in %d attempts\n",
>> +				valid, TASK_WORK_BUSY_ROUNDS, missed,
>> +				attempts_started);
>> +		if (!ASSERT_EQ(valid, TASK_WORK_BUSY_ROUNDS,
>> +			       "valid busy windows"))
>> +			goto cleanup;
>> +	}
> this isn't a bug, but could the three conditional fprintf() calls in the
> !hard_fail block be dropped?
>
> The summary fprintf() just above already reports valid,
> TASK_WORK_BUSY_ROUNDS, missed, attempts_started, stat_window and stat_total,
> and ASSERT_EQ() and ASSERT_GE() print the actual and expected values on
> failure.  The "busy coverage" message also re-tests the same condition as the
> ASSERT_EQ() that follows it.
>
>
> ---
> 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/38024809478