On Fri, Dec 08, 2023 at 02:02:34AM -0800, Souradeep Chakrabarti wrote: > Existing MANA design assigns IRQ to every CPU, including sibling > hyper-threads. This may cause multiple IRQs to be active simultaneously > in the same core and may reduce the network performance with RSS.
Can you add an IRQ distribution diagram to compare before/after behavior, similarly to what I did in the other email? > Improve the performance by assigning IRQ to non sibling CPUs in local > NUMA node. The performance improvement we are getting using ntttcp with > following patch is around 15 percent with existing design and approximately > 11 percent, when trying to assign one IRQ in each core across NUMA nodes, > if enough cores are present. How did you measure it? In the other email you said you used perf, can you show your procedure in details? > Suggested-by: Yury Norov <[email protected]> > Signed-off-by: Souradeep Chakrabarti <[email protected]> > --- [...] > .../net/ethernet/microsoft/mana/gdma_main.c | 92 +++++++++++++++++-- > 1 file changed, 83 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index 6367de0c2c2e..18e8908c5d29 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > @@ -1243,15 +1243,56 @@ void mana_gd_free_res_map(struct gdma_resource *r) > r->size = 0; > } > > +static int irq_setup(int *irqs, int nvec, int start_numa_node) > +{ > + int w, cnt, cpu, err = 0, i = 0; > + int next_node = start_numa_node; What for this? > + const struct cpumask *next, *prev = cpu_none_mask; > + cpumask_var_t curr, cpus; > + > + if (!zalloc_cpumask_var(&curr, GFP_KERNEL)) { > + err = -ENOMEM; > + return err; > + } > + if (!zalloc_cpumask_var(&cpus, GFP_KERNEL)) { free(curr); > + err = -ENOMEM; > + return err; > + } > + > + 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. > + cpumask_andnot(cpus, cpus, > topology_sibling_cpumask(cpu)); > + ++cnt; > + } > + } > + prev = next; > + } > +done: > + rcu_read_unlock(); > + free_cpumask_var(curr); > + free_cpumask_var(cpus); > + return err; > +} > + > static int mana_gd_setup_irqs(struct pci_dev *pdev) > { > - unsigned int max_queues_per_port = num_online_cpus(); > struct gdma_context *gc = pci_get_drvdata(pdev); > + unsigned int max_queues_per_port; > struct gdma_irq_context *gic; > unsigned int max_irqs, cpu; > - int nvec, irq; > + int start_irq_index = 1; > + int nvec, *irqs, irq; > int err, i = 0, j; > > + cpus_read_lock(); > + max_queues_per_port = num_online_cpus(); > if (max_queues_per_port > MANA_MAX_NUM_QUEUES) > max_queues_per_port = MANA_MAX_NUM_QUEUES; > > @@ -1261,6 +1302,14 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > nvec = pci_alloc_irq_vectors(pdev, 2, max_irqs, PCI_IRQ_MSIX); > if (nvec < 0) > return nvec; > + if (nvec <= num_online_cpus()) > + start_irq_index = 0; > + > + irqs = kmalloc_array((nvec - start_irq_index), sizeof(int), GFP_KERNEL); > + if (!irqs) { > + err = -ENOMEM; > + goto free_irq_vector; > + } > > gc->irq_contexts = kcalloc(nvec, sizeof(struct gdma_irq_context), > GFP_KERNEL); > @@ -1287,21 +1336,44 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > goto free_irq; > } > > - err = request_irq(irq, mana_gd_intr, 0, gic->name, gic); > - if (err) > - goto free_irq; > - > - cpu = cpumask_local_spread(i, gc->numa_node); > - irq_set_affinity_and_hint(irq, cpumask_of(cpu)); > + if (!i) { > + err = request_irq(irq, mana_gd_intr, 0, gic->name, gic); > + if (err) > + goto free_irq; > + > + /* If number of IRQ is one extra than number of online > CPUs, > + * then we need to assign IRQ0 (hwc irq) and IRQ1 to > + * same CPU. > + * Else we will use different CPUs for IRQ0 and IRQ1. > + * Also we are using cpumask_local_spread instead of > + * cpumask_first for the node, because the node can be > + * mem only. > + */ > + if (start_irq_index) { > + cpu = cpumask_local_spread(i, gc->numa_node); I already mentioned that: if i == 0, you don't need to spread, just pick 1st cpu from node. > + irq_set_affinity_and_hint(irq, cpumask_of(cpu)); > + } else { > + irqs[start_irq_index] = irq; > + } > + } else { > + irqs[i - start_irq_index] = irq; > + err = request_irq(irqs[i - start_irq_index], > mana_gd_intr, 0, > + gic->name, gic); > + if (err) > + goto free_irq; > + } > } > > + err = irq_setup(irqs, (nvec - start_irq_index), gc->numa_node); > + if (err) > + goto free_irq; > err = mana_gd_alloc_res_map(nvec, &gc->msix_resource); > if (err) > goto free_irq; > > gc->max_num_msix = nvec; > gc->num_msix_usable = nvec; > - > + cpus_read_unlock(); > return 0; > > free_irq: > @@ -1314,8 +1386,10 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > } > > kfree(gc->irq_contexts); > + kfree(irqs); > gc->irq_contexts = NULL; > free_irq_vector: > + cpus_read_unlock(); > pci_free_irq_vectors(pdev); > return err; > } > -- > 2.34.1
