>-----Original Message----- >From: Yury Norov <yury.norov@xxxxxxxxx> >Sent: Thursday, November 30, 2023 10:27 PM >To: Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxxxxxxxx> >Cc: Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxx>; Jakub Kicinski ><kuba@xxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang ><haiyangz@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Dexuan Cui ><decui@xxxxxxxxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; >pabeni@xxxxxxxxxx; Long Li <longli@xxxxxxxxxxxxx>; >sharmaajay@xxxxxxxxxxxxx; leon@xxxxxxxxxx; cai.huoqing@xxxxxxxxx; >ssengar@xxxxxxxxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; >linux-hyperv@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Paul Rosswurm ><paulros@xxxxxxxxxxxxx> >Subject: Re: [EXTERNAL] Re: [PATCH V2 net-next] net: mana: Assigning IRQ >affinity on HT cores > >[You don't often get email from yury.norov@xxxxxxxxx. Learn why this is >important at https://aka.ms/LearnAboutSenderIdentification ] > >On Thu, Nov 30, 2023 at 04:05:12AM -0800, Souradeep Chakrabarti wrote: >> On Wed, Nov 29, 2023 at 06:16:17PM -0800, Yury Norov wrote: >> > On Mon, Nov 27, 2023 at 09:36:38AM +0000, Souradeep Chakrabarti >wrote: >> > > >> > > >> > > >-----Original Message----- >> > > >From: Jakub Kicinski <kuba@xxxxxxxxxx> >> > > >Sent: Wednesday, November 22, 2023 5:19 AM >> > > >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; >> > > >pabeni@xxxxxxxxxx; Long Li <longli@xxxxxxxxxxxxx>; >> > > >sharmaajay@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 V2 net-next] net: mana: Assigning >> > > >IRQ affinity on HT cores >> > > > >> > > >On Tue, 21 Nov 2023 05:54:37 -0800 Souradeep Chakrabarti wrote: >> > > >> Existing MANA design assigns IRQ to every CPUs, including >> > > >> sibling hyper-threads in a core. This causes multiple IRQs to >> > > >> work on same CPU and may reduce the network performance with RSS. >> > > >> >> > > >> Improve the performance by adhering the configuration for RSS, >> > > >> which assigns IRQ on HT cores. >> > > > >> > > >Drivers should not have to carry 120 LoC for something as basic as >spreading IRQs. >> > > >Please take a look at include/linux/topology.h and if there's >> > > >nothing that fits your needs there - add it. That way other drivers can >reuse it. >> > > Because of the current design idea, it is easier to keep things >> > > inside the mana driver code here. As the idea of IRQ distribution here is : >> > > 1)Loop through interrupts to assign CPU 2)Find non sibling online >> > > CPU from local NUMA and assign the IRQs on them. >> > > 3)If number of IRQs is more than number of non-sibling CPU in that >> > > NUMA node, then assign on sibling CPU of that node. >> > > 4)Keep doing it till all the online CPUs are used or no more IRQs. >> > > 5)If all CPUs in that node are used, goto next NUMA node with CPU. >> > > Keep doing 2 and 3. >> > > 6) If all CPUs in all NUMA nodes are used, but still there are >> > > IRQs then wrap over from first local NUMA node and continue doing >> > > 2, 3 4 till all IRQs are assigned. >> > >> > Hi Souradeep, >> > >> > (Thanks Jakub for sharing this thread with me) >> > >> > If I understand your intention right, you can leverage the existing >> > cpumask_local_spread(). >> > >> > But I think I've got something better for you. The below series adds >> > a for_each_numa_cpu() iterator, which may help you doing most of the >> > job without messing with nodes internals. >> > >> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo >> > re.kernel.org%2Fnetdev%2FZD3l6FBnUh9vTIGc%40yury- >ThinkPad%2FT%2F&dat >> > >a=05%7C01%7Cschakrabarti%40microsoft.com%7C79dfb421db6f463627250 >8dbf >> > >1c5c19e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6383696 >04095521 >> > >996%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu >MzIiLCJB >> > >TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=pDUpYWo3K7 >uoz2q50GQ >> > 1UKuPF2PwFagiT5pwrXhQXPk%3D&reserved=0 >> > >> Thanks Yur and Jakub. I was trying to find this patch, but unable to find it on >that thread. >> Also in net-next I am unable to find it. Can you please tell, if it has been >committed? >> If not can you please point me out the correct patch for this macro. >> It will be really helpful. > >Try this branch. I just rebased it on top of bitmap-for-next, but didn't re-test. >You may need to exclude the "sched: drop for_each_numa_hop_mask()" patch. > >https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu >b.com%2Fnorov%2Flinux%2Fcommits%2Ffor_each_numa_cpu&data=05%7C0 >1%7Cschakrabarti%40microsoft.com%7C79dfb421db6f4636272508dbf1c5c1 >9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638369604095 >529277%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV >2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=W >wmd%2BvQS7YHIwFKyL9OLd8iYttJ4ZIqQyxU3Ex8UOkY%3D&reserved=0 > >> > By using it, the pseudocode implementing your algorithm may look >> > like this: >> > >> > unsigned int cpu, hop; >> > unsigned int irq = 0; >> > >> > again: >> > cpu = get_cpu(); >> > node = cpu_to_node(cpu); >> > cpumask_copy(cpus, cpu_online_mask); >> > >> > for_each_numa_cpu(cpu, hop, node, cpus) { >> > /* All siblings are the same for IRQ spreading purpose */ >> > irq_set_affinity_and_hint(irq, >> > topology_sibling_cpumask()); >> > >> > /* One IRQ per sibling group */ >> > cpumask_andnot(cpus, cpus, >> > topology_sibling_cpumask()); >> > >> > if (++irq == num_irqs) >> > break; >> > } >> > >> > if (irq < num_irqs) >> > goto again; >> > >> > (Completely not tested, just an idea.) >> > >> I have done similar kind of change for our driver, but constraint here >> is that total number of IRQs can be equal to the total number of >> online CPUs, in some setup. It is either equal to the number of online CPUs or >maximum 64 IRQs if online CPUs are more than that. > >Not sure I understand you. If you're talking about my proposal, there's >seemingly no constraints on number of CPUs/IRQs. > >> So my proposed change is following: >> >> +static int irq_setup(int *irqs, int nvec, int start_numa_node) { >> + cpumask_var_t node_cpumask; >> + int i, cpu, err = 0; >> + unsigned int next_node; >> + cpumask_t visited_cpus; >> + unsigned int start_node = start_numa_node; >> + i = 0; >> + if (!alloc_cpumask_var(&node_cpumask, GFP_KERNEL)) { >> + err = -ENOMEM; >> + goto free_mask; >> + } >> + cpumask_andnot(&visited_cpus, &visited_cpus, &visited_cpus); >> + start_node = 1; >> + for_each_next_node_with_cpus(start_node, next_node) { > >If your goal is to maximize locality, this doesn't seem to be correct. >for_each_next_node_with_cpus() is based on next_node(), and so enumerates >nodes in a numerically increasing order. On real machines, it's possible that >numerically adjacent node is not the topologically nearest. > >To approach that, for every node kernel maintains a list of equally distant nodes >grouped into hops. You may likely want to use for_each_numa_hop_mask >iterator, which iterated over hops in increasing distance order, instead of >NUMA node numbers. > >But I would like to see for_each_numa_cpu() finally merged as a simpler and >nicer alternative. > >> + cpumask_copy(node_cpumask, cpumask_of_node(next_node)); >> + for_each_cpu(cpu, node_cpumask) { >> + cpumask_andnot(node_cpumask, node_cpumask, >> + topology_sibling_cpumask(cpu)); >> + irq_set_affinity_and_hint(irqs[i], cpumask_of(cpu)); >> + if(++i == nvec) >> + goto free_mask; >> + cpumask_set_cpu(cpu, &visited_cpus); >> + if (cpumask_empty(node_cpumask) && >cpumask_weight(&visited_cpus) < >> + nr_cpus_node(next_node)) { >> + cpumask_copy(node_cpumask, >cpumask_of_node(next_node)); >> + cpumask_andnot(node_cpumask, node_cpumask, >&visited_cpus); >> + cpu = cpumask_first(node_cpumask); >> + } >> + } >> + if (next_online_node(next_node) == MAX_NUMNODES) >> + next_node = first_online_node; >> + } >> +free_mask: >> + free_cpumask_var(node_cpumask); >> + return err; >> +} >> >> I can definitely use the for_each_numa_cpu() instead of my proposed >> for_each_next_node_with_cpus() macro here and that will make it cleaner. >> Thanks for the suggestion. > >Sure. > >Can you please share performance measurements for a solution you'll finally >choose? Would be interesting to compare different approaches. I have compared spreading IRQs across numa with IRQs spread inside local NUMA, and the performance was 14 percent better for later. I have shared the V4 patch with for_each_numa_hop_mask() macro. > >Thanks, >Yury
