| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZWi+94B+N03pItJl@yury-ThinkPad>
Date: Thu, 30 Nov 2023 08:57:27 -0800
From: Yury Norov <yury.norov@...il.com>
To: Souradeep Chakrabarti <schakrabarti@...ux.microsoft.com>
Cc: Souradeep Chakrabarti <schakrabarti@...rosoft.com>,
Jakub Kicinski <kuba@...nel.org>, KY Srinivasan <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
Dexuan Cui <decui@...rosoft.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
Long Li <longli@...rosoft.com>,
"sharmaajay@...rosoft.com" <sharmaajay@...rosoft.com>,
"leon@...nel.org" <leon@...nel.org>,
"cai.huoqing@...ux.dev" <cai.huoqing@...ux.dev>,
"ssengar@...ux.microsoft.com" <ssengar@...ux.microsoft.com>,
"vkuznets@...hat.com" <vkuznets@...hat.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
Paul Rosswurm <paulros@...rosoft.com>
Subject: Re: [EXTERNAL] Re: [PATCH V2 net-next] net: mana: Assigning IRQ
affinity on HT cores
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@...nel.org>
> > > >Sent: Wednesday, November 22, 2023 5:19 AM
> > > >To: Souradeep Chakrabarti <schakrabarti@...ux.microsoft.com>
> > > >Cc: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> > > ><haiyangz@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> > > ><decui@...rosoft.com>; davem@...emloft.net; edumazet@...gle.com;
> > > >pabeni@...hat.com; Long Li <longli@...rosoft.com>;
> > > >sharmaajay@...rosoft.com; leon@...nel.org; cai.huoqing@...ux.dev;
> > > >ssengar@...ux.microsoft.com; vkuznets@...hat.com; tglx@...utronix.de; linux-
> > > >hyperv@...r.kernel.org; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> > > >linux-rdma@...r.kernel.org; Souradeep Chakrabarti
> > > ><schakrabarti@...rosoft.com>; Paul Rosswurm <paulros@...rosoft.com>
> > > >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://lore.kernel.org/netdev/ZD3l6FBnUh9vTIGc@yury-ThinkPad/T/
> >
> 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://github.com/norov/linux/commits/for_each_numa_cpu
> > 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.
Thanks,
Yury
Powered by blists - more mailing lists