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

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

On Tue, Dec 12, 2023 at 05:18:31PM +0000, Souradeep Chakrabarti wrote:
> 
> 
> >-----Original Message-----
> >From: Yury Norov <yury.norov@gmail.com>
> >Sent: Tuesday, December 12, 2023 10:04 PM
> >To: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com>
> >Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> ><haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> ><decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> >kuba@kernel.org; pabeni@redhat.com; Long Li <longli@microsoft.com>;
> >leon@kernel.org; cai.huoqing@linux.dev; ssengar@linux.microsoft.com;
> >vkuznets@redhat.com; tglx@linutronix.de; linux-hyperv@vger.kernel.org;
> >netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> >rdma@vger.kernel.org; Souradeep Chakrabarti <schakrabarti@microsoft.com>;
> >Paul Rosswurm <paulros@microsoft.com>
> >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@gmail.com. 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.

OK, I see. Then just separate it:

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