Re: [PATCH 0/5] sched_ext: selftest infrastructure fixes

[Tao Cui <cui.tao@linux.dev>]

Hello Tejun,

在 2026/10/13 7:56, Tejun Heo 写道:
> Hello, Tao.
> 
> The following is a Claude-generated review.
> 
> On Fri, Oct 09, 2026 at 09:40:59PM +0800, Tao Cui wrote:
>>   1. selftests/sched_ext: Fix reload_loop error-path use-after-free
> 
> Looks good. Inside the if (err) block, SCX_FAIL() reads better than
> SCX_FAIL_IF(err, ...).
Done (v2 patch 1).
> 
>>   2. sched_ext: Add SCX_ECODE_RSN_CGROUP_OFFLINE to user_exit_info.h
> 
> Can you name the commit in the description? It's ebeca1f930ea ("sched_ext:
> Introduce cgroup sub-sched support").
Done (v2 patch 2), which also fixes the wrong attribution in the v1
description - I had guessed "lifetime notification support".
> 
>>   3. selftests/sched_ext: Fail loudly when EXIT_KIND is missing from
>>      BTF. EXIT_KIND() resolves scx_exit_kind values through
>>      __COMPAT_ENUM_OR_ZERO(), which maps a missing enum to 0 (==
>>      SCX_EXIT_NONE), so on kernels whose BTF lacks the enum every
>>      EXIT_KIND() comparison is meaningless.
> 
> struct scx_exit_info carries enum scx_exit_kind, so any BTF that loads the
> skeleton has the enum. What can be missing is an enumerator, and the four
> the tests use today exist on every sched_ext kernel. On a kernel missing
> one, the comparisons against SCX_EXIT_NONE get the right value from the
> fallback, the SCX_EQ()s against UNREG/UNREG_BPF/ERROR fail with a confusing
> message, and the only vacuous pass is the error poll in kick.c.
> 
Conceded - v1's premise was wrong, thanks for the correction.

> The new EXIT_KIND() is also SCX_KIND_VAL() with a different message. If
> you want to keep the patch, maybe define one in terms of the other?
> 
> With a return hidden in the macro, the remaining
> kind != EXIT_KIND(SCX_EXIT_NONE) sites (allowed_cpus, cgroup_nr_cpus,
> cyclic_kick_wait, enq_blocked) jump over their goto cleanup on a miss, and
> invalid_one() resolves error_kind after the gated worker is spawned.
> Converting those sites to UEI_EXITED() like the polling helpers and
> hoisting the lookup above the spawn would close that.
> 
> The <unistd.h> include is unused in this patch, and compat.h already pulls
> it in.
> 
Kept, but reshaped along exactly these lines (v2 patch 3): EXIT_KIND()
is now an alias of SCX_KIND_VAL() so there is a single implementation,
the four `kind != EXIT_KIND(SCX_EXIT_NONE)` sites are converted to
UEI_EXITED(), invalid_one() resolves error_kind before spawning the
gated worker, and cyclic_kick_wait no longer calls EXIT_KIND() at all.
The unused <unistd.h> include is dropped.

>>   4. selftests/sched_ext: Add SCX_ASSERT_ALIVE and adopt it.
> 
> Looks good. Can SCX_ASSERT_ALIVE() print uei.msg too? That's where the
> actual error text ends up. UEI_REPORT() already does that.
> 
Done (v2 patch 4) - the failure now prints kind, exit code and uei.msg.

>>   5. selftests/sched_ext: Bound exit waits and report reload_loop
>>      failures.
> 
> On timeout, exit.c returns with the link and the skel still open. The
> scheduler that didn't exit stays attached for the rest of the runner
> process, so every later test fails to attach with EBUSY and the real
> failure is buried. prog_run and hotplug destroy the link on their timeout
> paths. Can exit.c do the same, plus exit__destroy()?
> 
Done (v2 patch 5) - the timeout path now destroys the link and the
skeleton before failing.

> In reload_loop, SCX_FAIL_IF(unexpected_errno, ...) returns before the
> printf(), so the ", unexpected errno" arm is dead, and the EBUSY count then
> prints on every run. 2048 of 2048 failing is also a weak bar; "at least one
> success per thread" would say the same thing more sharply. Would you split
> the timed wait and the reload_loop accounting into two patches?
> 
Split (v2 patches 5/6). The dead arm and the always-on printf are gone;
failures are reported by the fail conditions themselves. The bar is now
per-thread: the test fails if either thread failed every attach, and
the failure message carries the EBUSY count.

> Nits: two stray blank lines after setup(), and the two #defines could be an
> enum.
> 
> Can you note the base and how the series was tested in the cover?
> 
Done - the v2 cover states the base commit and the testing.

The two issues sashiko flagged on 5/5 (exit.c leak on timeout, dead
reload_loop arm) and the one on 3/5 (EXIT_KIND hidden return in
invalid_one) are all covered by the above.

Testing: same setup as v1 - QEMU x86_64 VM on top of the series base;
full sched_ext selftest run plus the A/B checks for each behavioral
change (synthetic BTF without the enumerator for patch 3, injected
scheduler death for patch 4, a scheduler stuck in ops.init for the
bounded waits, reload stress with in-flight teardown for patch 6).
Results are in the v2 cover letter.

Thanks,
Tao

> Thanks.
>