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

[Kayra Cizmeci <kayracizmeci@gmail.com>]

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