Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Kayra Cizmeci <kayracizmeci@gmail.com>]

On Thu, Oct 22, 2026 at 01:47:05PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 21, 2026 at 09:39:52AM -0700, Josh Don wrote:
> > On Wed, Oct 21, 2026 at 7:50 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Wed, Oct 21, 2026 at 04:41:52PM +0300, Kayra Cizmeci wrote:
> > > > On Wed, Oct 21, 2026 at 03:32:59PM +0200, Vincent Guittot wrote:
> > > > > On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > > > > What is the actual semantics of cpu.idle nonsense? Why do we care about
> > > > > > this common ancestor?
> > > > >
> > > > > AFAICT, cpu.idle applied to one level and against entities at the same level
> > > > >
> > > >
> > > > Yeah. Like on here:
> > > >
> > > >     ROOT
> > > >      /\
> > > >     /  \
> > > >    /    \
> > > >   GA*   GB
> > > >    |    /\
> > > >    V   /  \
> > > >   TA  GC*  GD
> > > >       |    |
> > > >       V    V
> > > >       TB   TC
> > >
> > > (marked GA and GC with * to denote their 'idleness')
> > >
> > > > If GA is idle and GB is not, while GC is idle and were looking to TA and
> > > > TB we gotta choose TB. But if we just look to GC and GA we would choose
> > > > neither. .idle just applies to one level as Vincent says.
> > >
> > > Yeah, that's what we used to do, but the more I think about this, the
> > > less sense it makes!
> > >
> > > Josh added this (added to Cc now), but we have a number of Google folks
> > > on Cc already. It would be good to hear how this is actually used.
> > >
> > >
> > > So in your example TA and TB are both inside an idle group, and
> > > therefore one can argue their behaviour should be like !idle. But their
> > > common ancestor is root, and so the fact that they're contained in idle
> > > groups gets lost (which seems crazeh).
> > 
> > TA and TB have a matching se of GA and GB, since those entities are
> > siblings under the common ancestor. Since GB is not idle but GA is, I
> > would argue that TB should preempt TA.
> 
> So I find this 'weird', at the end both TA and TB are marked idle,
> albeit at different levels in the hierarchy. And yes, with the old code
> this would lead to TB preempting TA, but I really don't see why that's a
> good thing.
> 
> > Consider GA and GB as groups representing different users, and user GB
> > has internally decided to create an idle cgroup GC to adjust
> > competition between its own tasks (TB and TC). This should not affect
> > GB's overall preemption ability against other users.
> 
> It doesn't; TC is not idle and will preempt just fine. But by marking
> GB, and thus TB, as idle, we basically say we don't care about it, so
> preempting should also be irrelevant.
> 
> Consider this:
> 
>      ROOT
>       /\
>      /  \
>     /    \
>    GA*   GB
>     |    /\
>     V   /  \
>    TA  GC*  GD
>        |    |
>        v    V
>        GE   TB
>        /\
>       /  \
>      TC   TD
> 
> Suppose TC runs and TD wakes, with the strict LCA rules we'd find that
> neither is idle and have one preempt the other. Even though they're well
> part of an 'idle' sub-tree.

I'm late by 2 hours or smtg :-(.

If we do this, TA would be 1, and T{C,D} would be 1 too. But at the old
code we would choose one of T{C,D} since GB is the same level as GA and
GA is idle.

I think your case is really weird too. But can't we fix this with just
checking the two tasks are already in the same sub-tree and walk that
sub-tree to the top and if we find a idle one we just don't preempt?

We could also do this fastly with this cfs_rq->idle thingie.

> 
> > However in practice doing the exact computation to determine
> > preemption for relative idle properties (especially on wakeup) is
> > expensive and perhaps not strictly required. This is also why e.g. the
> > whole sched_idle_rq stuff is imprecise. We keep an overall tally of
> > the number of tasks that have a cpu.idle ancestor cgroup and use that
> > for wakeup placement, but if we were trying to be perfect we would
> > need to consider the matching_se stuff for that as well. What I'd say
> > there though is that imprecise placement is better than imprecise
> > preemption.
> > 
> > So overall, I would say the find_matching_se() stuff gives the more
> > ideal preemption decision; my first preference would be to keep it. It
> > avoids a regression with users who delegate cpu.idle internally within
> > their own cgroup hierarchy. FWIW wakeup preemption already does a
> > cgroup ancestor walk via update_curr; find_matching_se is similar in
> > spirit.
> 
> So its probably not the end of the world -- and we sort of still have
> one in put_prev_task_fair(), but still, I find the semantics weird.
> 
> > But to be clear I also don't think it would be terrible if we did
> > something like you suggest, cache whether a cgroup has _any_ cpu.idle
> > ancestor and then just make a single decision on wakeup comparing this
> > for the two tasks. The result of which is TC preempts T{AB}, but TA
> > and TB don't preempt each other. In which case I would suggest we
> > cache that as a part of updating cpu.idle so that we can avoid a
> > pointer chasing loop in se_is_idle().
> 
> Just so. Let me try and implement that, see how horrible it looks.
> 
> > Either of these two will at least also restore some semblance of
> > respecting cgroup cpu.idle in wakeup, as the flattening made it only
> > consider task level SCHED_IDLE.
> 
> Right; I agree the current situation is somewhat broken.
> 
> Something like so ...


I'm late by 2 hours or smtg :-(.

If we do this, TA would be 1, and T{C,D} would be 1 too. But at the old
code we would choose one of T{C,D} since GB is the same level as GA and
GA is idle.

I think your case is really weird too. But can't we fix this with just
checking the two tasks are already in the same sub-tree and walk that
sub-tree to the top and if we find a idle one we just don't preempt?

We could also do this fastly with this cfs_rq->idle thingie.

Still, this all wouldn't be needed if losing the difference between 
TA and T{C,D} is acceptable for us. Is it? Well IDK :-).

Thoughts?


> 
> ---
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b5546cd27dba..779fcd5a2830 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -15767,6 +15767,40 @@ int sched_group_set_shares(struct task_group *tg, unsigned long shares)
>  	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;
> +	long 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) {
> +		if (!se->on_rq)
> +			break;
> +
> +		cfs_rq_of(se)->h_nr_idle += task_delta;
> +	}
> +
> +	return 0;
> +}
> +
>  int sched_group_set_idle(struct task_group *tg, long idle)
>  {
>  	int i;
> @@ -15777,49 +15811,22 @@ int sched_group_set_idle(struct task_group *tg, long idle)
>  	if (idle < 0 || idle > 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&shares_mutex);
> -
> -	if (tg->idle == idle) {
> -		mutex_unlock(&shares_mutex);
> +	guard(mutex)(&shares_mutex);
> +	if (tg->idle == idle)
>  		return 0;
> -	}
>  
>  	tg->idle = idle;
>  
>  	for_each_possible_cpu(i) {
>  		struct rq *rq = cpu_rq(i);
> -		struct sched_entity *se = tg_se(tg, i);
> -		struct cfs_rq *grp_cfs_rq = tg_cfs_rq(tg, i);
> -		bool was_idle = cfs_rq_is_idle(grp_cfs_rq);
> -		long idle_task_delta;
> -		struct rq_flags rf;
>  
> -		rq_lock_irqsave(rq, &rf);
> +		guard(rq_lock_irqsave)(rq);
>  
> -		grp_cfs_rq->idle = idle;
> -		if (WARN_ON_ONCE(was_idle == cfs_rq_is_idle(grp_cfs_rq)))
> -			goto next_cpu;
> -
> -		idle_task_delta = grp_cfs_rq->h_nr_queued -
> -				  grp_cfs_rq->h_nr_idle;
> -		if (!cfs_rq_is_idle(grp_cfs_rq))
> -			idle_task_delta *= -1;
> -
> -		for_each_sched_entity(se) {
> -			struct cfs_rq *cfs_rq = cfs_rq_of(se);
> -
> -			if (!se->on_rq)
> -				break;
> -
> -			cfs_rq->h_nr_idle += idle_task_delta;
> -
> -			/* Already accounted at parent level and above. */
> -			if (cfs_rq_is_idle(cfs_rq))
> -				break;
> -		}
> -
> -next_cpu:
> -		rq_unlock_irqrestore(rq, &rf);
> +		struct tg_set_idle_struct tsi = {
> +			.idle = idle ? 1 : -1,
> +			.cpu = i,
> +		};
> +		walk_tg_tree_from(tg, tg_nop, tg_set_idle_up, &tsi);
>  	}
>  
>  	/* Idle groups have minimum weight. */
> @@ -15828,7 +15835,6 @@ int sched_group_set_idle(struct task_group *tg, long idle)
>  	else
>  		__sched_group_set_shares(tg, NICE_0_LOAD);
>  
> -	mutex_unlock(&shares_mutex);
>  	return 0;
>  }
>  
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 499a5649582e..06bcee5bb89f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -751,7 +751,7 @@ struct cfs_rq {
>  	struct list_head	leaf_cfs_rq_list;
>  	struct task_group	*tg;	/* Group that "owns" this runqueue */
>  
> -	/* Locally cached copy of our task_group's idle value */
> +	/* Sum of all ancestor tg->idle. */
>  	int			idle;
>  
>  # ifdef CONFIG_CFS_BANDWIDTH