Re: [PATCH 1/2] sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `SCX_RQ_CLK_VALID` can be permanently stranded as 1 during `scx_root_disable()`, causing the next SCX scheduler to read a massively outdated clock via `scx_bpf_now()`.
--

--- Patch [1]: [PATCH 1/2] sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable() ---
commit c8d4a8f1095e17e1c662f243f67711302652b61f
Author: Tejun Heo <[email protected]>

sched_ext: Drop unlocked scx_rq_clock_invalidate() from scx_root_disable()

This commit drops the unlocked scx_rq_clock_invalidate() call during
scx_root_disable(), relying instead on the rq lock cycle and unlock
path to clear the clock valid flags.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 18183062f751c..40c9467d8b186 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -6162,12 +6162,13 @@ static void scx_root_disable(struct scx_sched *sch)
>  	percpu_up_write(&scx_fork_rwsem);
>  
>  	/*
> -	 * Invalidate all the rq clocks to prevent getting outdated
> -	 * rq clocks from a previous scx scheduler.
> +	 * Re-balance the dl_server bandwidth reservations: detach ext_server
> +	 * (no more sched_ext tasks) and reinstate fair_server if it was
> +	 * previously detached because we were running in full mode.
>  	 *
> -	 * Also re-balance the dl_server bandwidth reservations: detach
> -	 * ext_server (no more sched_ext tasks) and reinstate fair_server if it
> -	 * was previously detached because we were running in full mode.
> +	 * The rq lock cycle also invalidates each rq's clock (rq_unpin_lock()
> +	 * clears SCX_RQ_CLK_VALID on unlock), preventing the next enable from
> +	 * seeing outdated rq clocks from this scheduler.

[Severity: High]
This problem wasn't introduced by this patch, but is it possible for the
SCX_RQ_CLK_VALID flag to be permanently stranded as 1 during
scx_root_disable()?

The commit message and comment note that rq_unpin_lock() clears the flag
on unlock, and __scx_enabled is turned off later. However, if a concurrent
CPU takes the rq lock after the cycle but before __scx_enabled is disabled,
it seems a race can occur:

CPU N (Normal scheduling):
Takes rq lock
update_rq_clock() sets SCX_RQ_CLK_VALID (since __scx_enabled is still true)

CPU 0 (scx_root_disable):
static_branch_disable(&__scx_enabled);

CPU N (Normal scheduling):
Releases rq lock -> rq_unpin_lock() -> scx_rq_clock_invalidate()
scx_rq_clock_invalidate() in kernel/sched/sched.h checks:
    if (!scx_enabled())
        return;

Because scx_enabled() now evaluates to false, the function returns early and
fails to clear SCX_RQ_CLK_VALID.

When a new BPF scheduler is loaded later, early hooks like ops.select_cpu
that run without holding the rq lock might call scx_bpf_now() and see
SCX_RQ_CLK_VALID == 1. Will this cause them to use a massively outdated
clock from the previous scheduler's run?

>  	 *
>  	 * Unlike the enable path, this runs on a recovery path that cannot
>  	 * fail, so we use dl_server_swap_bw() to atomically free ext_server's
> @@ -6180,8 +6181,6 @@ static void scx_root_disable(struct scx_sched *sch)
>  	for_each_possible_cpu(cpu) {
>  		struct rq *rq = cpu_rq(cpu);
>  
> -		scx_rq_clock_invalidate(rq);
> -
>  		scoped_guard(rq_lock_irqsave, rq) {
>  			update_rq_clock(rq);
>  			if (was_switched_all) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1