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

[Yury Norov <ynorov@nvidia.com>]

On Wed, Apr 01, 2026 at 12:09:52PM +0200, Valentin Schneider wrote:
> On 28/03/26 17:42, Yury Norov wrote:
> > Please no kernel doc syntax out of kernel doc blocks.
> >
> 
> Why not? It's straight to the point and not uncommon in sched code.

Because it's weird to see machine-readable marks in a text for human.
In that particular file, I see it in:
 - NUMA topology huge comment block for 'level' of hops, which is not
   even a variable, so misleading;
 - in annotation to sched_numa_find_closest() - should be converted to
   kernel doc and fixed; and 
 - in an inline comment in build_sched_domain:
   /* Fixup, ensure @sd has at least @child CPUs. */

So, a single more or less matching case in a 3k LOCs file.

I'll send a patch.
 
> >>  	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.

hop_masks is the bsearch output, so pretty straightforward to me. If
you think it's vague, maybe just pick a better name?

The k.w should be explained in struct __cmp_key description. And just
in case, it's an output field containing the number of CPUs in a given
hop, matching the user-provided 'cpus' mask.

> >> +	 */
> >>  	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]);