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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 12, 2023 at 05:18:31PM +0000, Souradeep Chakrabarti wrote:
> 
> 
> >-----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.

OK, I see. Then just separate it:

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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux