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

[PATCH] 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
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 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] sched_ext: don't deliver duplicate ops.cgroup_set_idle() for same value

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch references a non-existent struct member `tg->scx.sched_idle`, which causes a build failure.
--

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

sched_ext: don't deliver duplicate ops.cgroup_set_idle() for same value

This commit modifies scx_group_set_idle() to prevent delivering duplicate
cgroup transition callbacks to the BPF scheduler. It checks if the new idle
value differs from the existing value before invoking the callback.

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

> 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 this code build successfully when CONFIG_EXT_GROUP_SCHED is enabled?
It appears tg->scx.sched_idle is referencing a non-existent member in
struct scx_task_group.

Looking at include/linux/sched/ext.h, the scx_task_group struct defines
the field simply as idle:

struct scx_task_group {
    ...
    bool idle;
    ...
};

Should this condition check tg->scx.idle != idle instead?

>  		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/20260901031101.731943-1-cui.tao@linux.dev?part=1

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

[Andrea Righi]

Hi Tao,

On Tue, Sep 01, 2026 at 11:11:01AM +0800, Tao Cui wrote:
> 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

This link seems broken, I think the right one is:

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

> Signed-off-by: Tao Cui <cuitao@kylinos.cn>

Other than that looks good to me.

Reviewed-by: Andrea Righi <arighi@nvidia.com>

Thanks,
-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] sched_ext: don't deliver duplicate ops.cgroup_set_idle() for same value

[Tao Cui]

Hi Andrea,

在 2026/9/1 15:25, Andrea Righi 写道:
> Hi Tao,
> 
> On Tue, Sep 01, 2026 at 11:11:01AM +0800, Tao Cui wrote:
>> 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
> 
> This link seems broken, I think the right one is:
> 
> Link: https://lore.kernel.org/r/b53c61a1-4d7d-4232-941f-d48b0563d4ed@linux.dev
> 

Thanks! Somehow my vim seems to have eaten the `@linux.dev` part of the Message-ID. I'll fix the Link tag in the next revision.

Thanks for the review!

Best,
Tao

>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> 
> Other than that looks good to me.
> 
> Reviewed-by: Andrea Righi <arighi@nvidia.com>
> 
> Thanks,
> -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
>>