Re: [PATCH v2 1/4] sched/fair: Check CPU capacity before comparing group types during load balance

[Ricardo Neri <ricardo.neri-calderon@linux.intel.com>]

On Wed, May 06, 2026 at 11:38:31AM +0100, Christian Loehle wrote:
> On 4/29/26 22:19, Ricardo Neri wrote:
> > update_sd_pick_busiest() may incorrectly select a fully_busy group as the
> > busiest group when its per-CPU capacity exceeds that of the destination
> > CPU. This happens because the type of busiest group is initialized to
> > group_has_spare and allows the fully_busy group to win the type comparison.
> > 
> > update_sd_pick_busiest() should not choose a candidate scheduling group
> > with at most one runnable task if its per-CPU capacity is greater than that
> > of the destination CPU. Such a check already exists, but it is done too
> > late: after the type comparison, preventing a subsequent fully_busy group
> > of equal per-CPU capacity from being correctly selected.
> > 
> > Move this check to occur before comparing group types.
> > 
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v1:
> >  * Added a note clarifying that SMT and SD_ASYM_CPUCAPACITY are mutually
> >    exclusive. (Tim)
> >  * Kept parentheses around bitwise operators for clarity.
> >  * Rewrote patch description for clarity.
> > ---
> >  kernel/sched/fair.c | 25 ++++++++++++++-----------
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 728965851842..0dbed82aa63f 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -10788,6 +10788,20 @@ static bool update_sd_pick_busiest(struct lb_env *env,
> >  	     sds->local_stat.group_type != group_has_spare))
> >  		return false;
> >  
> > +	/*
> > +	 * Candidate sg has no more than one task per CPU and has higher
> > +	 * per-CPU capacity. Migrating tasks to less capable CPUs may harm
> > +	 * throughput. Maximize throughput, power/energy consequences are not
> > +	 * considered.
> > +	 *
> > +	 * Systems with SMT are unaffected, as asymmetric capacity is not set
> > +	 * in such case.
> > +	 */
> > +	if ((env->sd->flags & SD_ASYM_CPUCAPACITY) &&
> > +	    (sgs->group_type <= group_fully_busy) &&
> > +	    (capacity_greater(sg->sgc->min_capacity, capacity_of(env->dst_cpu))))
> > +		return false;
> > +
> >  	if (sgs->group_type > busiest->group_type)
> >  		return true;
> >  
> > @@ -10890,17 +10904,6 @@ static bool update_sd_pick_busiest(struct lb_env *env,
> >  		break;
> >  	}
> >  
> > -	/*
> > -	 * Candidate sg has no more than one task per CPU and has higher
> > -	 * per-CPU capacity. Migrating tasks to less capable CPUs may harm
> > -	 * throughput. Maximize throughput, power/energy consequences are not
> > -	 * considered.
> > -	 */
> > -	if ((env->sd->flags & SD_ASYM_CPUCAPACITY) &&
> > -	    (sgs->group_type <= group_fully_busy) &&
> > -	    (capacity_greater(sg->sgc->min_capacity, capacity_of(env->dst_cpu))))
> > -		return false;
> > -
> >  	return true;
> >  }
> >  
> > 
> 
> I think it deserves a Fixes, but nonetheless:

I will add this tag.

> Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Thank you!

> 
> I've CCed Andrea, just because of this SMT -> !SD_ASYM_CPUCAPACITY currently
> being up for debate...

Ah, I missed that patchset. I will take a look. For now I will leave the
comment. It can always be updated later.