Re: [PATCH] sched/topology: optimize sched_numa_find_nth_cpu()

[Valentin Schneider]

On 28/03/26 17:42, Yury Norov wrote:
> On Thu, Mar 26, 2026 at 08:45:25PM +0100, Valentin Schneider wrote:
>> @@ -2284,16 +2289,16 @@ static int hop_cmp(const void *a, const void *b)
>>  }
>>  
>>  /**
>> - * sched_numa_find_nth_cpu() - given the NUMA topology, find the Nth closest CPU
>> - *                             from @cpus to @cpu, taking into account distance
>> - *                             from a given @node.
>> + * sched_numa_find_nth_cpu() - given the NUMA topology, find the @nth_cpu in
>> + *                             @cpus reachable from @node in the least amount
>> + *                             of hops.
>>   * @cpus: cpumask to find a cpu from
>> - * @cpu: CPU to start searching
>> - * @node: NUMA node to order CPUs by distance
>> + * @nth_cpu: CPU offset to search for
>> + * @node: NUMA node to start the search from
>>   *
>>   * Return: cpu, or nr_cpu_ids when nothing found.
>>   */
>> -int sched_numa_find_nth_cpu(const struct cpumask *cpus, int cpu, int node)
>> +int sched_numa_find_nth_cpu(const struct cpumask *cpus, int nth_cpu, int node)
>
> If you think that 'cpu' is confusing, then 'nth_cpu' would be even
> more confusing: you're searching for the nth_cpu, and you pass it as a
> parameter.
>

Right

> Let's name it just num or idx, with the reasoning: 
>
> 1. This is not CPU (contrary to cpumask_next(cpu), for example); and
> 2. This is just an index in a numa-based distance enumeration.
>
> And the description should reflect that, like:
>
>  @idx: index of a CPU to find in a node-based distance CPU enumeration
>

Index works for me, that's how cpumask_local_spread() calls it.

>>  {
>>  	struct __cmp_key k = { .cpus = cpus, .cpu = cpu };
>>  	struct cpumask ***hop_masks;
>> @@ -2315,8 +2320,21 @@ int sched_numa_find_nth_cpu(const struct cpumask *cpus, int cpu, int node)
>>  	hop_masks = bsearch(&k, k.masks, sched_domains_numa_levels, sizeof(k.masks[0]), hop_cmp);
>>  	if (!hop_masks)
>>  		goto unlock;
>> +	/*
>> +	 * bsearch returned sched_domains_numa_masks[hop], with @hop being the
>> +	 * smallest amount of hops it takes to reach an @nth_cpu from @node.
>> +	 */
>
> Please no kernel doc syntax out of kernel doc blocks.
>

Why not? It's straight to the point and not uncommon in sched code.

>>  	hop = hop_masks	- k.masks;
>>  
>> +	/*
>> +	 * @hop is constructed by hop_cmp() such that sched_domains_numa_masks[hop][node]
>
> hop_cmp() doesn't construct, it's a comparator for bsearch, which searches for
> the nearest hop containing at least N CPUs.
>
>> +	 * spans enough CPUs to return an @nth_cpu, and sched_domains_numa_masks[hop-1][node]
>> +	 * doesn't.
>> +	 *
>> +	 * Get a cpumask without the CPUs from sched_domains_numa_masks[hop-1][node]
>> +	 * subtract how many CPUs that contains (@k.w), and fetch our @nth_cpu from
>> +	 * the resulting mask.
>
> It explains only hop != NULL case. This sentence confuses more than
> explain, to me. The below code is quite self-explaining. 
>

My goal was to allow the reader to have a gist of how @hop_masks and @k are
arranged upon returning from bsearch() without having to dig down to how
bsearch()+hop_cmp() will interact.

Right now nothing tells you what @k.w is unless you go dig for it.

>> +	 */
>>  	ret = hop ?
>>  		cpumask_nth_and_andnot(cpu - k.w, cpus, k.masks[hop][node], k.masks[hop-1][node]) :
>>  		cpumask_nth_and(cpu, cpus, k.masks[0][node]);