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

[Souradeep Chakrabarti]

On Mon, Dec 11, 2023 at 06:00:22AM -0800, Yury Norov wrote:
> On Sun, Dec 10, 2023 at 10:53:23PM -0800, Souradeep Chakrabarti wrote:
> > On Fri, Dec 08, 2023 at 01:53:51PM -0800, Yury Norov wrote:
> > > Few more nits
> > > 
> > > On Fri, Dec 08, 2023 at 06:03:40AM -0800, Yury Norov wrote:
> > > > 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 <yury.norov@xxxxxxxxx>
> > > > > Signed-off-by: Souradeep Chakrabarti <schakrabarti@xxxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > 
> > > > [...]
> > > > 
> > > > >  .../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)) {
> > > 
> > > alloc_cpumask_var() here and below, because you initialize them by
> > > copying
> > I have used zalloc here as prev gets initialized after the first hop, before that
> > it may contain unwanted values, which may impact cpumask_andnot(curr, next, prev).
> > Regarding curr I will change it to alloc_cpumask_var().
> > Please let me know if that sounds right.
> 
> What? prev is initialized at declaration:
Yes, I will remove the zalloc and will change it to alloc in V6.
Thanks for pointing.
>         
>         const struct cpumask *next, *prev = cpu_none_mask;