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

[Peter Zijlstra <peterz@infradead.org>]

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.

> 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 ...

---
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