Re: [PATCH v2] sched_ext: don't deliver duplicate ops.cgroup_set_idle() for same value

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lockless state check introduces a Time-of-Check to Time-of-Use (TOCTOU) race that can permanently desynchronize the BPF scheduler from the actual cgroup state.
- [Low] Compilation failure due to use of non-existent field `sched_idle` in `struct scx_task_group`.
--

commit 47d6f0af51f3086b39745cdfc5f053f908dbd0c7
Author: Tao Cui <cuitao@kylinos.cn>

sched_ext: don't deliver duplicate ops.cgroup_set_idle() for same value
    
This commit avoids sending duplicate cgroup_set_idle() callbacks to the BPF
scheduler by checking if the cgroup's idle state has actually changed.

Link: https://lore.kernel.org/r/b53c61a1-4d7d-4232-941f-d48b0563d4ed@linux.dev

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index c539d15cda637..c72d9780830b8 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -4930,7 +4930,8 @@ void scx_group_set_idle(struct task_group *tg, bool idle)
>  	percpu_down_read(&scx_cgroup_ops_rwsem);
>  	sch = scx_tg_knob_sched(tg);
>  
> -	if (scx_cgroup_enabled && sch && SCX_HAS_OP(sch, cgroup_set_idle))
> +	if (scx_cgroup_enabled && sch && SCX_HAS_OP(sch, cgroup_set_idle) &&
> +	    tg->scx.sched_idle != idle)

[Severity: Low]
Does sched_idle exist in struct scx_task_group? It looks like this might
cause a compile failure when building with CONFIG_SCHED_CLASS_EXT and
CONFIG_EXT_GROUP_SCHED enabled. Should this be checking tg->scx.idle instead?

[Severity: High]
Is it possible for concurrent writes to cpu.idle to permanently desynchronize
the BPF scheduler from the actual cgroup state here?

Since this state check is performed locklessly before the callback is sent,
if Thread A writes 1 and Thread B concurrently writes 0, could Thread B
evaluate the new check while the state is still 0, and falsely skip sending
its callback?

If Thread A then sends its callback for 1, and Thread B subsequently updates
the state to 0, the BPF scheduler might think the state is 1 while the
internal tracking state is 0.

If a user tries to manually fix this by echoing 0 again, wouldn't the
lockless check see the internal state is already 0 and skip the corrective
callback, leaving the BPF scheduler trapped in the wrong state?

>  		SCX_CALL_OP(sch, cgroup_set_idle, NULL, tg_cgrp(tg), idle);
>  
>  	/* Update the task group's idle state */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901124347.755904-1-cui.tao@linux.dev?part=1