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

[Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxx>]

>-----Original Message-----
>From: Yury Norov <yury.norov@xxxxxxxxx>
>Sent: Tuesday, December 12, 2023 10:04 PM
>To: Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxxxxxxxx>
>Cc: 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; Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxx>;
>Paul Rosswurm <paulros@xxxxxxxxxxxxx>
>Subject: [EXTERNAL] Re: [PATCH V5 net-next] net: mana: Assigning IRQ affinity on
>HT cores
>
>[Some people who received this message don't often get email from
>yury.norov@xxxxxxxxx. Learn why this is important at
>https://aka.ms/LearnAboutSenderIdentification ]
>
>> > > > > +     rcu_read_lock();
>> > > > > +     for_each_numa_hop_mask(next, next_node) {
>> > > > > +             cpumask_andnot(curr, next, prev);
>> > > > > +             for (w = cpumask_weight(curr), cnt = 0; cnt < w; ) {
>> > > > > +                     cpumask_copy(cpus, curr);
>> > > > > +                     for_each_cpu(cpu, cpus) {
>> > > > > +                             irq_set_affinity_and_hint(irqs[i],
>topology_sibling_cpumask(cpu));
>> > > > > +                             if (++i == nvec)
>> > > > > +                                     goto done;
>> > > >
>> > > > Think what if you're passed with irq_setup(NULL, 0, 0).
>> > > > That's why I suggested to place this check at the beginning.
>> > > >
>> > > irq_setup() is a helper function for mana_gd_setup_irqs(), which
>> > > already takes care of no NULL pointer for irqs, and 0 number of interrupts can
>not be passed.
>> > >
>> > > nvec = pci_alloc_irq_vectors(pdev, 2, max_irqs, PCI_IRQ_MSIX); if
>> > > (nvec < 0)
>> > >   return nvec;
>> >
>> > I know that. But still it's a bug. The common convention is that if
>> > a 0-length array is passed to a function, it should not dereference
>> > the pointer.
>> >
>> I will add one if check in the begining of irq_setup() to verify the
>> pointer and the nvec number.
>
>Yes you can, but what for? This is an error anyways, and you don't care about early
>return. So instead of adding and bearing extra logic, I'd just swap 2 lines of existing
>code.
Problem with the code you had proposed is shown below:

> ./a.out
 i is 1
 i is 2
 i is 3
 i is 4
 i is 5
 i is 6
 i is 7
 i is 8
 i is 9
 i is 10
in done
lisatest ~
> cat test3.c
#include<stdio.h>

main() {
        int i = 0, cur, nvec = 10;
        for (cur = 0; cur < 20; cur++) {
                if (i++ == nvec)
                        goto done;
                printf(" i is %d\n", i);
        }
done:                                                                                                                                                                                                                                                                                  
printf("in done\n");
}

So now it is because post increment operator in i++,
For that reason in the posposed code we will hit irqs[nvec], which may cause crash, as size of
irqs is nvec.

Now if we preincrement, then we will loop correctly, but nvec == 0 check will not happen.

Like here with preincrement in above code we are not hitting (i == nvec) .
> ./a.out
 i is 1
 i is 2
 i is 3
 i is 4
 i is 5
 i is 6
 i is 7
 i is 8
 i is 9
in done

So with preincrement if we want the check for nvec == 0, we will need the check with extra if condition
before the loop.