Re: [PATCH v3 3/4] net: mana: Allow irq_setup() to skip cpus for affinity

[Yury Norov <yury.norov@gmail.com>]

On Wed, May 14, 2025 at 04:53:34AM +0000, Michael Kelley wrote:
> > -static int irq_setup(unsigned int *irqs, unsigned int len, int node)
> > +static int irq_setup(unsigned int *irqs, unsigned int len, int node,
> > +		     bool skip_first_cpu)
> >  {
> >  	const struct cpumask *next, *prev = cpu_none_mask;
> >  	cpumask_var_t cpus __free(free_cpumask_var);
> > @@ -1303,9 +1304,20 @@ static int irq_setup(unsigned int *irqs, unsigned int len, int node)
> >  		while (weight > 0) {
> >  			cpumask_andnot(cpus, next, prev);
> >  			for_each_cpu(cpu, cpus) {
> > +				/*
> > +				 * if the CPU sibling set is to be skipped we
> > +				 * just move on to the next CPUs without len--
> > +				 */
> > +				if (unlikely(skip_first_cpu)) {
> > +					skip_first_cpu = false;
> > +					goto next_cpumask;
> > +				}
> > +
> >  				if (len-- == 0)
> >  					goto done;
> > +
> >  				irq_set_affinity_and_hint(*irqs++, topology_sibling_cpumask(cpu));
> > +next_cpumask:
> >  				cpumask_andnot(cpus, cpus, topology_sibling_cpumask(cpu));
> >  				--weight;
> >  			}
> 
> With a little bit of reordering of the code, you could avoid the need for the "next_cpumask"
> label and goto statement.  "continue" is usually cleaner than a "goto". Here's what I'm thinking:
> 
> 		for_each_cpu(cpu, cpus) {
> 			cpumask_andnot(cpus, cpus, topology_sibling_cpumask(cpu));
> 			--weight;

cpumask_andnot() is O(N), and before it was conditional on 'len == 0',
so we didn't do that on the very last step. Your version has to do that.
Don't know how important that is for real workloads. Shradha maybe can
measure it...

> 
> 			If (unlikely(skip_first_cpu)) {
> 				skip_first_cpu = false;
> 				continue;
> 			}
> 
> 			If (len-- == 0)
> 				goto done;
> 
> 			irq_set_affinity_and_hint(*irqs++, topology_sibling_cpumask(cpu));
> 		}
> 
> I wish there were some comments in irq_setup() explaining the overall intention of
> the algorithm. I can see how the goal is to first assign CPUs that are local to the current
> NUMA node, and then expand outward to CPUs that are further away. And you want
> to *not* assign both siblings in a hyper-threaded core.

I wrote this function, so let me step in.

The intention is described in the corresponding commit message:

  Souradeep investigated that the driver performs faster if IRQs are
  spread on CPUs with the following heuristics:
  
  1. No more than one IRQ per CPU, if possible;
  2. NUMA locality is the second priority;
  3. Sibling dislocality is the last priority.
  
  Let's consider this topology:
  
  Node            0               1
  Core        0       1       2       3
  CPU       0   1   2   3   4   5   6   7
  
  The most performant IRQ distribution based on the above topology
  and heuristics may look like this:
  
  IRQ     Nodes   Cores   CPUs
  0       1       0       0-1
  1       1       1       2-3
  2       1       0       0-1
  3       1       1       2-3
  4       2       2       4-5
  5       2       3       6-7
  6       2       2       4-5
  7       2       3       6-7

> But I can't figure out what
> "weight" is trying to accomplish. Maybe this was discussed when the code first
> went in, but I can't remember now. :-(

The weight here is to implement the heuristic discovered by Souradeep:
NUMA locality is preferred over sibling dislocality. 

The outer for_each() loop resets the weight to the actual number of
CPUs in the hop. Then inner for_each() loop decrements it by the
number of sibling groups (cores) while assigning first IRQ to each
group. 

Now, because NUMA locality is more important, we should walk the
same set of siblings and assign 2nd IRQ, and it's implemented by the
medium while() loop. So, we do like this unless the number of IRQs
assigned on this hop will not become equal to number of CPUs in the
hop (weight == 0). Then we switch to the next hop and do the same
thing.

Hope that helps.

Thanks,
Yury