Re: [PATCH 2/3] sched/fair: Make cfs_rq::idle the sum of ancestor tg::idle

[Kayra Cizmeci]

On Thu, Nov 05, 2026 at 05:55:27PM +0100, Peter Zijlstra wrote:
> Rather than keeping cfs_rq::idle as a copy of tg::idle, make it the sum of
> all ancestor tg::idle. This makes it trivial to tell if the current cgroup
> is marked idle somewhere along its ancestral path.
> 
> This makes se_id_idle() behave differently. Notably is will now be true
> when there is a idle cgroup anywhere in the ancestor path. This
> addresses an issue where preemption of cgroup::idle was overlooked when
> flattening the runqueue.

How? The diff doesn't touches se_is_idle(). se_is_idle() just looks to
the policy for tasks:

static int se_is_idle(struct sched_entity *se)
{
	if (entity_is_task(se))
		return task_has_idle_policy(task_of(se));
	return cfs_rq_is_idle(group_cfs_rq(se));
}

And the issue in the preemption path is in the entity_is_task() path.

	cse_is_idle = se_is_idle(se);
	pse_is_idle = se_is_idle(pse);

	nse = se;
	/*
	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
	 * in the inverse case).
	 */
	if (cse_is_idle && !pse_is_idle)
		goto preempt;

	update_curr_fair(rq);

	if (cse_is_idle != pse_is_idle)
		goto update;



Or am I getting something wrong?


> 
> Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  kernel/sched/core.c  |    2 -
>  kernel/sched/debug.c |    1 
>  kernel/sched/fair.c  |   73 ++++++++++++++++++++++++++++++++-------------------
>  kernel/sched/sched.h |    4 +-
>  4 files changed, 50 insertions(+), 30 deletions(-)
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8970,7 +8970,7 @@ void __init sched_init(void)
>  		rq->nr_running = 0;
>  		rq->calc_load_active = 0;
>  		rq->calc_load_update = jiffies + LOAD_FREQ;
> -		init_cfs_rq(&rq->cfs);
> +		init_cfs_rq(&rq->cfs, NULL);
>  		init_rt_rq(&rq->rt);
>  		init_dl_rq(&rq->dl);
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -1090,6 +1090,7 @@ void print_cfs_rq(struct seq_file *m, in
>  	SEQ_printf(m, "  .%-30s: %d\n", "h_nr_runnable", cfs_rq->h_nr_runnable);
>  	SEQ_printf(m, "  .%-30s: %d\n", "h_nr_queued", cfs_rq->h_nr_queued);
>  	SEQ_printf(m, "  .%-30s: %d\n", "h_nr_idle", cfs_rq->h_nr_idle);
> +	SEQ_printf(m, "  .%-30s: %d\n", "idle", cfs_rq->idle);
>  	SEQ_printf(m, "  .%-30s: %ld\n", "load", cfs_rq->load.weight);
>  	SEQ_printf(m, "  .%-30s: %lu\n", "load_avg",
>  			cfs_rq->avg.load_avg);
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -15594,8 +15594,10 @@ static void set_next_task_fair(struct rq
>  		hrtick_start_fair(rq, p);
>  }
>  
> -void init_cfs_rq(struct cfs_rq *cfs_rq)
> +void init_cfs_rq(struct cfs_rq *cfs_rq, struct cfs_rq *parent)
>  {
> +	if (parent)
> +		cfs_rq->idle = parent->idle;

AFAICT this creates a race:

Let's say we are creating a group under GA named GB.

We first call sched_create_group(), that calls alloc_fair_sched_group()
and that calls init_cfs_rq(). If the parent groups idle state changes
before GB gets into place in something like this:


* GA is not idle GB idle = 0

* GA gets idle in the window

* GB setells in

* GA then gets idle = 0 again


Now this happens:

GB idle = -1 and GA idle = 0.

So below, in tg_set_idle_up() the code things something changed and
h_nr_idle starts to corrupt.


Or am I missing something?


>  	cfs_rq->tasks_timeline = RB_ROOT_CACHED;
>  	cfs_rq->zero_vruntime = (u64)(-(1LL << 20));
>  	raw_spin_lock_init(&cfs_rq->removed.lock);
> @@ -15646,7 +15648,7 @@ int alloc_fair_sched_group(struct task_g
>  			goto err;
>  
>  		se = tg_se(tg, i);
> -		init_cfs_rq(cfs_rq);
> +		init_cfs_rq(cfs_rq, tg_cfs_rq(parent, i));
>  		init_tg_cfs_entry(tg, cfs_rq, se, i, tg_se(parent, i));
>  		init_entity_runnable_average(se);
>  	}
> @@ -15780,6 +15782,43 @@ int sched_group_set_shares(struct task_g
>  	return ret;
>  }
>  
> +struct tg_set_idle_struct {
> +	long idle;
> +	int cpu;
> +};
> +
> +static int tg_set_idle_up(struct task_group *tg, void *data)
> +{
> +	struct tg_set_idle_struct *tsi = data;
> +	struct cfs_rq *grp_cfs_rq = tg_cfs_rq(tg, tsi->cpu);
> +	struct sched_entity *se = tg_se(tg, tsi->cpu);
> +	int old_idle = grp_cfs_rq->idle;
> +	int task_delta;
> +
> +	grp_cfs_rq->idle += tsi->idle;
> +
> +	if (!!old_idle == !!grp_cfs_rq->idle)
> +		return 0;
> +
> +	task_delta = grp_cfs_rq->h_nr_queued - grp_cfs_rq->h_nr_idle;
> +	if (!task_delta)
> +		return 0;
> +
> +	task_delta *= tsi->idle;
> +	for_each_sched_entity(se) {
> +		struct cfs_rq *cfs_rq = cfs_rq_of(se);
> +
> +		if (!se->on_rq)
> +			break;
> +
> +		cfs_rq->h_nr_idle += task_delta;
> +
> +		if (cfs_rq_is_idle(cfs_rq))
> +			break;
> +	}
> +	return 0;
> +}


Cheers,


Kayra :-)