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++);
