RE: [PATCH net-next] net: mana: Assigning IRQ affinity on HT cores

[Souradeep Chakrabarti]

>-----Original Message-----
>From: Michael Kelley <mhklinux@xxxxxxxxxxx>
>Sent: Thursday, November 16, 2023 11:47 AM
>To: Michael Kelley <mhklinux@xxxxxxxxxxx>; Souradeep Chakrabarti
><schakrabarti@xxxxxxxxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
>Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Dexuan Cui
><decui@xxxxxxxxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
>kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Long Li <longli@xxxxxxxxxxxxx>;
>leon@xxxxxxxxxx; cai.huoqing@xxxxxxxxx; ssengar@xxxxxxxxxxxxxxxxxxx;
>vkuznets@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx;
>netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
>rdma@xxxxxxxxxxxxxxx
>Cc: Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxx>; Paul Rosswurm
><paulros@xxxxxxxxxxxxx>
>Subject: [EXTERNAL] RE: [PATCH net-next] net: mana: Assigning IRQ affinity on HT
>cores
>
>[Some people who received this message don't often get email from
>mhklinux@xxxxxxxxxxx. Learn why this is important at
>https://aka.ms/LearnAboutSenderIdentification ]
>
>From: Michael Kelley <mhklinux@xxxxxxxxxxx> Sent: Wednesday, November 15,
>2023 9:26 PM
>> >
>> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
>> > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
>> > index 6367de0c2c2e..839be819d46e 100644
>> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
>> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
>> > @@ -1243,13 +1243,115 @@ void mana_gd_free_res_map(struct
>> > gdma_resource *r)
>> >     r->size = 0;
>> >  }
>> >
>> > +static void cpu_mask_set(cpumask_var_t *filter_mask, cpumask_var_t
>> > **filter_mask_list)
>> > +{
>> > +   unsigned int core_count = 0, cpu;
>> > +   cpumask_var_t *filter_mask_list_tmp;
>> > +
>> > +   BUG_ON(!filter_mask || !filter_mask_list);
>>
>> This check seems superfluous. The function is invoked from only one
>> call site in the code below, and that site call site can easily ensure
>> that it doesn't pass a NULL value for either parameter.
Fixed it in V2 patch.
>>
>> > +   filter_mask_list_tmp = *filter_mask_list;
>> > +   cpumask_copy(*filter_mask, cpu_online_mask);
>> > +   /* for each core create a cpumask lookup table,
>> > +    * which stores all the corresponding siblings
>> > +    */
>> > +   for_each_cpu(cpu, *filter_mask) {
>> > +
>>       BUG_ON(!alloc_cpumask_var(&filter_mask_list_tmp[core_count],
>> GFP_KERNEL));
>>
>> Don't panic if a memory allocation fails.  Must back out, clean up,
>> and return an error.
>>
>> > +           cpumask_or(filter_mask_list_tmp[core_count],
>filter_mask_list_tmp[core_count],
>> > +                      topology_sibling_cpumask(cpu));
>>
>> alloc_cpumask_var() does not zero out the returned cpumask.  So the
>> cpumask_or() is operating on uninitialized garbage.  Use
>> zalloc_cpumask_var() to get a cpumask initialized to zero.
>>
>> But why is the cpumask_or() being done at all?  Couldn't this just be
>> a cpumask_copy()?  In that case, alloc_cpumask_var() is OK.
>>
>> > +           cpumask_andnot(*filter_mask, *filter_mask,
>topology_sibling_cpumask(cpu));
>> > +           core_count++;
>> > +   }
>> > +}
>> > +
>> > +static int irq_setup(int *irqs, int nvec) {
>> > +   cpumask_var_t filter_mask;
>> > +   cpumask_var_t *filter_mask_list;
>> > +   unsigned int cpu_first, cpu, irq_start, cores = 0;
>> > +   int i, core_count = 0, numa_node, cpu_count = 0, err = 0;
>> > +
>> > +   BUG_ON(!alloc_cpumask_var(&filter_mask, GFP_KERNEL));
>>
>> Again, don't panic if a memory allocation fails.  Must back out and
>> return an error.
>>
>> > +   cpus_read_lock();
>> > +   cpumask_copy(filter_mask, cpu_online_mask);
>>
>> For readability, I'd suggest adding a blank line before any of the
>> multi-line comments below.
>>
>> > +   /* count the number of cores
>> > +    */
>> > +   for_each_cpu(cpu, filter_mask) {
>> > +           cpumask_andnot(filter_mask, filter_mask,
>topology_sibling_cpumask(cpu));
>> > +           cores++;
>> > +   }
>> > +   filter_mask_list = kcalloc(cores, sizeof(cpumask_var_t), GFP_KERNEL);
>> > +   if (!filter_mask_list) {
>> > +           err = -ENOMEM;
>> > +           goto free_irq;
>
>One additional macro-level comment.  Creating and freeing the filter_mask_list is
>a real pain.  But it is needed to identify which CPUs in a core are available to have
>an IRQ assigned to them.  So let me suggest a modified approach to meeting that
>need.
>
>1) Count the number of cores just as before.
>
>2) Then instead of allocating filter_mask_list, allocate an array of integers, with one
>array entry per core.  Call the array core_id_list.
>Somewhat like the code in cpu_mask_set(), populate the array with the CPU
>number of the first CPU in each core.  The populating only needs to be done once,
>so the code can be inline in irq_setup().
>It doesn't need to be in a helper function like cpu_mask_set().
>
>3) Allocate a single cpumask_var_t local variable that is initialized to a copy of
>cpu_online_mask.  Call it avail_cpus.  This local variable indicates which CPUs
>(across all cores) are available to have an IRQ assigned.
>
>4) At the beginning of the "for" loop over nvec, current code has:
>
>        cpu_first = cpumask_first(filter_mask_list[core_count]);
>
>Instead, do:
>
>        cpu_first = cpumask_first_and(&avail_cpus,
>                        topology_sibling_cpumask(core_id_list[core_count]));
>
>The above code effectively creates on-the-fly the cpumask previously stored in
>filter_mask_list, and finds the first CPU as before.
>
>5) When a CPU is assigned an IRQ, clear that CPU in avail_cpus, not in the
>filter_mask_list entry.
>
>6) When the NUMA node wraps around and current code calls cpu_mask_set(),
>instead reinitialize avail_cpus to cpu_online_mask like in my #3 above.
>
>In summary, with this approach filter_mask_list isn't needed at all.  The messiness
>of allocating and freeing an array of cpumask's goes away.  I haven't coded it, but I
>think the result will be simpler and less error prone.
>
Changed it in V2.
>Michael
>
>> > +   }
>> > +   /* if number of cpus are equal to max_queues per port, then
>> > +    * one extra interrupt for the hardware channel communication.
>> > +    */
>>
>> Note that higher level code may set nvec based on the # of online
>> CPUs, but until the cpus_read_lock is held, the # of online CPUs can
>> change. So nvec might have been determined when the # of CPUs was 8,
>> but 2 CPUs could go offline before the cpus_read_lock is obtained.  So
>> nvec could be bigger than just 1 more than num_online_cpus().  I'm not
>> sure how to handle the check below to special case the hardware
>> communication channel.  But realize that the main "for" loop below
>> could end up assigning multiple IRQs to the same CPU if nvec >
>> num_online_cpus().
>>
>> > +   if (nvec - 1 == num_online_cpus()) {
>> > +           irq_start = 1;
>> > +           cpu_first = cpumask_first(cpu_online_mask);
>> > +           irq_set_affinity_and_hint(irqs[0], cpumask_of(cpu_first));
>> > +   } else {
>> > +           irq_start = 0;
>> > +   }
>> > +   /* reset the core_count and num_node to 0.
>> > +    */
>> > +   core_count = 0;
>> > +   numa_node = 0;
>> > +   cpu_mask_set(&filter_mask, &filter_mask_list);
>>
>> FWIW, passing filter_mask as the first argument above was confusing to
>> me until I realized that the value of filter_mask doesn't matter --
>> it's just to use the memory allocated for filter_mask.  Maybe a
>> comment would help.  And ditto for the invocation of cpu_mask_set()
>> further down.
Fixed it in V2.
>>
>> > +   /* for each interrupt find the cpu of a particular
>> > +    * sibling set and if it belongs to the specific numa
>> > +    * then assign irq to it and clear the cpu bit from
>> > +    * the corresponding sibling list from filter_mask_list.
>> > +    * Increase the cpu_count for that node.
>> > +    * Once all cpus for a numa node is assigned, then
>> > +    * move to different numa node and continue the same.
>> > +    */
>> > +   for (i = irq_start; i < nvec; ) {
>> > +           cpu_first = cpumask_first(filter_mask_list[core_count]);
>> > +           if (cpu_first < nr_cpu_ids && cpu_to_node(cpu_first) ==
>> > + numa_node) {
>>
>> Note that it's possible to have a NUMA node with zero online CPUs.
>> It could be a NUMA node that was originally a memory-only NUMA node
>> and never had any CPUs, or the NUMA node could have had CPUs, but they
>> were all later taken offline.  Such a NUMA node is still considered to
>> be online because it has memory, but it has no online CPUs.
>>
>> If this code ever sets "numa_node" to such a NUMA node, the "for" loop
>> will become an infinite loop because the "if"
>> statement above will never find a CPU that is assigned to "numa_node".
>>
>> > +                   irq_set_affinity_and_hint(irqs[i], cpumask_of(cpu_first));
>> > +                   cpumask_clear_cpu(cpu_first, filter_mask_list[core_count]);
>> > +                   cpu_count = cpu_count + 1;
>> > +                   i = i + 1;
>> > +                   /* checking if all the cpus are used from the
>> > +                    * particular node.
>> > +                    */
>> > +                   if (cpu_count == nr_cpus_node(numa_node)) {
>> > +                           numa_node = numa_node + 1;
>> > +                           if (numa_node == num_online_nodes()) {
>> > +                                   cpu_mask_set(&filter_mask,
>> > + &filter_mask_list);
>>
>> The second and subsequent calls to cpu_mask_set() will leak any memory
>> previously allocated by alloc_cpumask_var() in cpu_mask_set().
>>
>> I agree with Haiyang's comment about starting with a NUMA node other
>> than NUMA node zero.  But if you do so, note that testing for
>> wrap-around at num_online_nodes() won't be equivalent to testing for
>> getting back to the starting NUMA node.
>> You really want to run cpu_mask_set() again only when you get back to
>> the starting NUMA node.
Fixed it in V2.
>>
>> > +                                   numa_node = 0;
>> > +                           }
>> > +                           cpu_count = 0;
>> > +                           core_count = 0;
>> > +                           continue;
>> > +                   }
>> > +           }
>> > +           if ((core_count + 1) % cores == 0)
>> > +                   core_count = 0;
>> > +           else
>> > +                   core_count++;
>>
>> The if/else could be more compactly written as:
>>
>>               if (++core_count == cores)
>>                       core_count = 0;
>>
>> > +   }
>> > +free_irq:
>> > +   cpus_read_unlock();
>> > +   if (filter_mask)
>>
>> This check for non-NULL filter_mask is incorrect and might not even
>> compile if CONFIG_CPUMASK_OFFSTACK=n.  For testing purposes, you
>> should make sure to build and run with each
>> option:  with CONFIG_CPUMASK_OFFSTACK=y and also
>> CONFIG_CPUMASK_OFFSTACK=n.
Fixed it in V2.
>>
>> It's safe to just call free_cpumask_var() without the check.
>>
>> > +           free_cpumask_var(filter_mask);
>> > +   if (filter_mask_list) {
>> > +           for (core_count = 0; core_count < cores; core_count++)
>> > +                   free_cpumask_var(filter_mask_list[core_count]);
>> > +           kfree(filter_mask_list);
>> > +   }
>> > +   return err;
>> > +}