Re: [PATCH v2 04/13] sched/fair: Remove magic hardcoded margin in fits_capacity()

[Qais Yousef]

On 9/23/26 07:54, Zhan Xusheng wrote:
> On 05/04/26 02:59, Qais Yousef wrote:
>> -#define fits_capacity(cap, max)	((cap) * 1280 < (max) * 1024)
>> +static inline bool fits_capacity(unsigned long util, int cpu)
>> +{
>> +	return util < cpu_rq(cpu)->fits_capacity_threshold;
>> +}
> 
> This does not hold on current tip anymore: fits_capacity() has picked up
> a second caller that does not pass a capacity, and the new signature
> accepts it silently.
> 
> kernel/sched/fair.c, invalid_llc_nr():
> 
> 	return !fits_capacity((READ_ONCE(grp->nr_running_avg) * cpu_smt_num_threads),
> 			(scale * per_cpu(sd_llc_size, cpu)));
> 
> The second argument is a scaled count of the CPUs in an LLC. sd_llc_size
> is a per-CPU int and scale is an int, so the product binds to the new @cpu
> parameter with no conversion and no warning, and the test becomes
> 
> 	cpu_rq(scale * per_cpu(sd_llc_size, cpu))->fits_capacity_threshold
> 
> With the defaults (CONFIG_SCHED_CACHE=y, sysctl_sched_cache_user=1,
> llc_aggr_tolerance=1) scale is 1, so this is cpu_rq(sd_llc_size), which is
> not a valid CPU id on a machine with a single LLC. invalid_llc_nr() is
> called from account_mm_sched(), task_cache_work() and
> can_migrate_llc_task().
> 
> Separating the two uses first makes 04/13 safe to apply. Something like
> the below, against tip/sched/urgent (3cb0243767fd, where the cache-aware
> fixes have just landed; sched/core still has the older mm->sc_stat form).
> It is a no functional change, since x * 1280 < y * 1024 is equivalent to
> x * 100 < y * 80. Built with CONFIG_SCHED_CACHE=y and =n, no new warnings.
> 
> I can post it as a standalone patch if you would rather not carry it in
> the series.

Thanks for catching this and reporting it. I am preparing an updated version and
would have missed this tbh.

I'm happy for them to be split; but not happy we have a new user with magic
numbers :( I am trying to remove these numbers, so seeing more added is unfortunate.

If you don't mind posting a patch to split and discuss the details that'd be great.

Thanks!

--
Qais Yousef

> 
> ---
>  kernel/sched/fair.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 57360f5cdde4..97ce56fcaf85 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1526,6 +1526,21 @@ static bool exceed_llc_capacity(struct sched_cache_group *grp, int cpu)
>  	return false;
>  }
>  
> +/*
> + * The margin used when comparing the number of a process' active threads
> + * with the number of CPUs in an LLC.
> + *
> + * Mirrors the ~20% margin of fits_capacity(), but is kept separate because
> + * fits_capacity() describes a utilization versus CPU capacity relation,
> + * which this is not.
> + *
> + * (default: ~80% of the LLC's CPUs)
> + */
> +static inline bool fits_llc_nr(u64 nr_threads, unsigned int nr_cpus)
> +{
> +	return nr_threads * 100 < (u64)nr_cpus * 80;
> +}
> +
>  static bool invalid_llc_nr(struct sched_cache_group *grp, struct task_struct *p,
>  			   int cpu)
>  {
> @@ -1542,8 +1557,8 @@ static bool invalid_llc_nr(struct sched_cache_group *grp, struct task_struct *p,
>  	if (scale == INT_MAX)
>  		return false;
>  
> -	return !fits_capacity((READ_ONCE(grp->nr_running_avg) * cpu_smt_num_threads),
> -			(scale * per_cpu(sd_llc_size, cpu)));
> +	return !fits_llc_nr(READ_ONCE(grp->nr_running_avg) * cpu_smt_num_threads,
> +			    scale * per_cpu(sd_llc_size, cpu));
>  }
>  
>  /*