[PATCH 0/3] sched/fair: Fix cgroup::idle preemption

[PATCH 0/3] sched/fair: Fix cgroup::idle preemption

[Peter Zijlstra]

Hi!

so I finally got around to looking at this again with a fresh mind and an LLM
generated testcase. It appears to actually work now.

I'm still not convinced on the whole find_matching_se() thing; at the end of
the day we mark a subtree 'idle', which is akin to saying 'dont care', so then
actually caring about how it ends up preempting on a different level is weird
to me.

As such, I have Vincent's patch last; modified to use the new idle state as
optimization.

[PATCH 1/3] sched/fair: Use guard in sched_group_set_idle()

[Peter Zijlstra]

Cleanup the code by using guards; prepares for further changes in this function.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c |   16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -15790,12 +15790,9 @@ int sched_group_set_idle(struct task_gro
 	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;
 
@@ -15805,13 +15802,12 @@ int sched_group_set_idle(struct task_gro
 		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;
+			continue;
 
 		idle_task_delta = grp_cfs_rq->h_nr_queued -
 				  grp_cfs_rq->h_nr_idle;
@@ -15830,9 +15826,6 @@ int sched_group_set_idle(struct task_gro
 			if (cfs_rq_is_idle(cfs_rq))
 				break;
 		}
-
-next_cpu:
-		rq_unlock_irqrestore(rq, &rf);
 	}
 
 	/* Idle groups have minimum weight. */
@@ -15841,7 +15834,6 @@ int sched_group_set_idle(struct task_gro
 	else
 		__sched_group_set_shares(tg, NICE_0_LOAD);
 
-	mutex_unlock(&shares_mutex);
 	return 0;
 }
 

Re: [PATCH 1/3] sched/fair: Use guard in sched_group_set_idle()

[Kayra Cizmeci]

Hi Peter,

On Thu, Nov 05, 2026 at 05:55:26PM +0100, Peter Zijlstra wrote:

> Cleanup the code by using guards; prepares for further changes in this function.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/fair.c |   16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -15790,12 +15790,9 @@ int sched_group_set_idle(struct task_gro
>  	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;
>  
> @@ -15805,13 +15802,12 @@ int sched_group_set_idle(struct task_gro
>  		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;
> +			continue;
>  
>  		idle_task_delta = grp_cfs_rq->h_nr_queued -
>  				  grp_cfs_rq->h_nr_idle;
> @@ -15830,9 +15826,6 @@ int sched_group_set_idle(struct task_gro
>  			if (cfs_rq_is_idle(cfs_rq))
>  				break;
>  		}
> -
> -next_cpu:
> -		rq_unlock_irqrestore(rq, &rf);
>  	}
>  
>  	/* Idle groups have minimum weight. */
> @@ -15841,7 +15834,6 @@ int sched_group_set_idle(struct task_gro
>  	else
>  		__sched_group_set_shares(tg, NICE_0_LOAD);
>  
> -	mutex_unlock(&shares_mutex);
>  	return 0;
>  }
>  
> 
> 

LGTM.

Reviewed-by: Kayra Cizmeci <kayracizmeci@gmail.com>


Cheers,

Kayra

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

[Peter Zijlstra]

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.

Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 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;
 	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;
+}
+
 int sched_group_set_idle(struct task_group *tg, long idle)
 {
 	int i;
@@ -15798,34 +15837,14 @@ int sched_group_set_idle(struct task_gro
 
 	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;
 
 		guard(rq_lock_irqsave)(rq);
 
-		grp_cfs_rq->idle = idle;
-		if (WARN_ON_ONCE(was_idle == cfs_rq_is_idle(grp_cfs_rq)))
-			continue;
-
-		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;
-		}
+		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. */
--- 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
@@ -3520,7 +3520,7 @@ print_numa_stats(struct seq_file *m, int
 		 unsigned long tpf, unsigned long gsf, unsigned long gpf);
 #endif /* CONFIG_NUMA_BALANCING */
 
-extern void init_cfs_rq(struct cfs_rq *cfs_rq);
+extern void init_cfs_rq(struct cfs_rq *cfs_rq, struct cfs_rq *parent);
 extern void init_rt_rq(struct rt_rq *rt_rq);
 extern void init_dl_rq(struct dl_rq *dl_rq);
 

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@infradead.org>
> ---
>  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 :-)

[RFC][PATCH 3/3] sched/eevdf: Fix sched idle task group preemption

[Peter Zijlstra]

From: Vincent Guittot <vincent.guittot@linaro.org>

Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
we need to parse the hierarchy to check for sched_idle task group.

Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
Reported-by: Jakub Okoński <jakub@okonski.org>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/58dc8e8a-7260-43ce-aebb-8d9c29a6afc9@okonski.org/
Link: https://patch.msgid.link/20261019182622.1584454-1-vincent.guittot@linaro.org
---

Changes since v1:
- fix prototype and cleanup unsued variable

 kernel/sched/fair.c |   87 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 74 insertions(+), 13 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -433,6 +433,38 @@ static inline struct sched_entity *paren
 	return se->parent;
 }
 
+static void
+find_matching_se(struct sched_entity **se, struct sched_entity **pse)
+{
+	int se_depth, pse_depth;
+
+	/*
+	 * preemption test can be made between sibling entities who are in the
+	 * same cfs_rq i.e who have a common parent. Walk up the hierarchy of
+	 * both tasks until we find their ancestors who are siblings of common
+	 * parent.
+	 */
+
+	/* First walk up until both entities are at same depth */
+	se_depth = (*se)->depth;
+	pse_depth = (*pse)->depth;
+
+	while (se_depth > pse_depth) {
+		se_depth--;
+		*se = parent_entity(*se);
+	}
+
+	while (pse_depth > se_depth) {
+		pse_depth--;
+		*pse = parent_entity(*pse);
+	}
+
+	while (!is_same_group(*se, *pse)) {
+		*se = parent_entity(*se);
+		*pse = parent_entity(*pse);
+	}
+}
+
 static int tg_is_idle(struct task_group *tg)
 {
 	return tg->idle > 0;
@@ -476,6 +508,11 @@ static inline struct sched_entity *paren
 	return NULL;
 }
 
+static inline void
+find_matching_se(struct sched_entity **se, struct sched_entity **pse)
+{
+}
+
 static inline int tg_is_idle(struct task_group *tg)
 {
 	return 0;
@@ -10130,7 +10167,40 @@ preempt_sync(struct rq *rq, int wake_fla
 }
 
 /*
- * Preempt the current task with a newly woken task if needed:
+ * Preempt an idle entity in favor of a non-idle entity (and don't preempt
+ * in the inverse case).
+ */
+static inline enum preempt_wakeup_action
+wakeup_preempt_sched_idle(struct sched_entity *cse, struct sched_entity *pse)
+{
+	int cse_is_idle, pse_is_idle;
+
+	cse_is_idle = se_is_idle(cse);
+	pse_is_idle = se_is_idle(pse);
+
+	if (cse_is_idle || pse_is_idle) {
+		find_matching_se(&cse, &pse);
+		WARN_ON_ONCE(!pse);
+
+		cse_is_idle = se_is_idle(cse);
+		pse_is_idle = se_is_idle(pse);
+
+		/*
+		 * 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)
+			return PREEMPT_WAKEUP_RESCHED;
+
+		if (cse_is_idle != pse_is_idle)
+			return PREEMPT_WAKEUP_NONE;
+	}
+
+	return PREEMPT_WAKEUP_PICK;
+}
+
+/*
+ * Preempt the current task with a newly woken task if needed.
  */
 static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_flags)
 {
@@ -10138,7 +10208,6 @@ static void wakeup_preempt_fair(struct r
 	struct task_struct *donor = rq->donor;
 	struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
 	struct cfs_rq *cfs_rq = &rq->cfs;
-	int cse_is_idle, pse_is_idle;
 
 	/*
 	 * XXX Getting preempted by higher class, try and find idle CPU?
@@ -10175,22 +10244,14 @@ static void wakeup_preempt_fair(struct r
 	if (!sched_feat(WAKEUP_PREEMPTION))
 		return;
 
-	WARN_ON_ONCE(!pse);
-
-	cse_is_idle = se_is_idle(se);
-	pse_is_idle = se_is_idle(pse);
+	preempt_action = wakeup_preempt_sched_idle(se, 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)
+	if (preempt_action == PREEMPT_WAKEUP_RESCHED)
 		goto preempt;
 
 	update_curr_fair(rq);
 
-	if (cse_is_idle != pse_is_idle)
+	if (preempt_action == PREEMPT_WAKEUP_NONE)
 		goto update;
 
 	/*

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

[Kayra Cizmeci]

On Thu, Nov 05, 2026 at 05:55:28PM +0100, Peter Zijlstra wrote:
> From: Vincent Guittot <vincent.guittot@linaro.org>
> 
> Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
> we need to parse the hierarchy to check for sched_idle task group.
> 
> Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
> Reported-by: Jakub Okoński <jakub@okonski.org>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lore.kernel.org/all/58dc8e8a-7260-43ce-aebb-8d9c29a6afc9@okonski.org/
> Link: https://patch.msgid.link/20261019182622.1584454-1-vincent.guittot@linaro.org
> ---
> 
> Changes since v1:
> - fix prototype and cleanup unsued variable
> 
>  kernel/sched/fair.c |   87 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 74 insertions(+), 13 deletions(-)
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -433,6 +433,38 @@ static inline struct sched_entity *paren
>  	return se->parent;
>  }
>  
> +static void
> +find_matching_se(struct sched_entity **se, struct sched_entity **pse)
> +{
> +	int se_depth, pse_depth;
> +
> +	/*
> +	 * preemption test can be made between sibling entities who are in the
> +	 * same cfs_rq i.e who have a common parent. Walk up the hierarchy of
> +	 * both tasks until we find their ancestors who are siblings of common
> +	 * parent.
> +	 */
> +
> +	/* First walk up until both entities are at same depth */
> +	se_depth = (*se)->depth;
> +	pse_depth = (*pse)->depth;
> +
> +	while (se_depth > pse_depth) {
> +		se_depth--;
> +		*se = parent_entity(*se);
> +	}
> +
> +	while (pse_depth > se_depth) {
> +		pse_depth--;
> +		*pse = parent_entity(*pse);
> +	}
> +
> +	while (!is_same_group(*se, *pse)) {
> +		*se = parent_entity(*se);
> +		*pse = parent_entity(*pse);
> +	}
> +}
> +
>  static int tg_is_idle(struct task_group *tg)
>  {
>  	return tg->idle > 0;
> @@ -476,6 +508,11 @@ static inline struct sched_entity *paren
>  	return NULL;
>  }
>  
> +static inline void
> +find_matching_se(struct sched_entity **se, struct sched_entity **pse)
> +{
> +}
> +
>  static inline int tg_is_idle(struct task_group *tg)
>  {
>  	return 0;
> @@ -10130,7 +10167,40 @@ preempt_sync(struct rq *rq, int wake_fla
>  }
>  
>  /*
> - * Preempt the current task with a newly woken task if needed:
> + * Preempt an idle entity in favor of a non-idle entity (and don't preempt
> + * in the inverse case).
> + */
> +static inline enum preempt_wakeup_action
> +wakeup_preempt_sched_idle(struct sched_entity *cse, struct sched_entity *pse)
> +{
> +	int cse_is_idle, pse_is_idle;
> +
> +	cse_is_idle = se_is_idle(cse);
> +	pse_is_idle = se_is_idle(pse);
> +
> +	if (cse_is_idle || pse_is_idle) {

We pass tasks to wakeup_preempt_sched_idle() (below) so this
se_is_idle() would just look to policy.

So even if they are in a idle group, if they don't have idle policy
themselves it just returns false.


Or am I getting something wrong?


> +		find_matching_se(&cse, &pse);
> +		WARN_ON_ONCE(!pse);
> +
> +		cse_is_idle = se_is_idle(cse);
> +		pse_is_idle = se_is_idle(pse);


Look here:

    ROOT
      |
     GA*
     /\
    /  \
   GB  GC*
   |    |
   TA   TB

Normally in the old code if TB is waking and TA is running we wouldn't
preempt TA. But because se_is_idle() returns true if idle is bigger than
0, both GB and GC is considered the same (if my other issue is true,
walk doesn't even starts so we don't come even come here)

Is this knowingly like this?

> +
> +		/*
> +		 * 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)
> +			return PREEMPT_WAKEUP_RESCHED;
> +
> +		if (cse_is_idle != pse_is_idle)
> +			return PREEMPT_WAKEUP_NONE;
> +	}
> +
> +	return PREEMPT_WAKEUP_PICK;
> +}
> +
> +/*
> + * Preempt the current task with a newly woken task if needed.
>   */
>  static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_flags)
>  {
> @@ -10138,7 +10208,6 @@ static void wakeup_preempt_fair(struct r
>  	struct task_struct *donor = rq->donor;
>  	struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
>  	struct cfs_rq *cfs_rq = &rq->cfs;
> -	int cse_is_idle, pse_is_idle;
>  
>  	/*
>  	 * XXX Getting preempted by higher class, try and find idle CPU?
> @@ -10175,22 +10244,14 @@ static void wakeup_preempt_fair(struct r
>  	if (!sched_feat(WAKEUP_PREEMPTION))
>  		return;
>  
> -	WARN_ON_ONCE(!pse);
> -
> -	cse_is_idle = se_is_idle(se);
> -	pse_is_idle = se_is_idle(pse);
> +	preempt_action = wakeup_preempt_sched_idle(se, 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)
> +	if (preempt_action == PREEMPT_WAKEUP_RESCHED)
>  		goto preempt;
>  
>  	update_curr_fair(rq);
>  
> -	if (cse_is_idle != pse_is_idle)
> +	if (preempt_action == PREEMPT_WAKEUP_NONE)
>  		goto update;
>  
>  	/*
> 
> 


Cheers,

Kayra