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

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

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

ops.cgroup_set_idle() is documented to be invoked when a cgroup
transitions between idle and non-idle states, and scx_group_set_weight()
already skips value-preserving writes. scx_group_set_idle() delivers
every write unconditionally, so rewriting an already-correct cpu.idle
value feeds the BPF scheduler a transition callback each time, which
toggle- or accounting-based schedulers miscount. Mirror the weight
guard and only deliver on an actual change.

Verified with a probe scheduler printing each callback: rewriting
cpu.idle=1 twice on an already-idle cgroup delivered two callbacks
before and none after.

Fixes: 347ed2d566da ("sched/ext: Implement cgroup_set_idle() callback")
Link: https://lore.kernel.org/r/b53c61a1-4d7d-4232-941f-d48b0563d4ed@linux.dev
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Reviewed-by: Andrea Righi <arighi@nvidia.com>
---
v1 -> v2: Fix the Link: msgid (missing @linux.dev, Andrea).

 kernel/sched/ext/ext.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 8041c87a3562..8b3625107b72 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -4933,7 +4933,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)
 		SCX_CALL_OP(sch, cgroup_set_idle, NULL, tg_cgrp(tg), idle);
 
 	/* Update the task group's idle state */
-- 
2.43.0

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

[sashiko-bot]

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

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

[Tejun Heo]

> ops.cgroup_set_idle() is documented to be invoked when a cgroup
> transitions between idle and non-idle states, and scx_group_set_weight()
> already skips value-preserving writes. scx_group_set_idle() delivers
> every write unconditionally, so rewriting an already-correct cpu.idle
> value feeds the BPF scheduler a transition callback each time, which
> toggle- or accounting-based schedulers miscount. Mirror the weight
> guard and only deliver on an actual change.

Applied to sched_ext/for-7.3-fixes with the subject capitalized and the
new comparison changed to tg->scx.idle, which is the field's name there:

-	    tg->scx.sched_idle != idle)
+	    tg->scx.idle != idle)

The rename to tg->scx.sched_idle is on for-7.4 and the for-next merge
switches the comparison back.

Thanks.

--
tejun