netdev - Re: [PATCH v3 4/4] net: mana: Allocate MSI-X vectors dynamically

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250515051055.GC5681@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date: Wed, 14 May 2025 22:10:55 -0700
From: Shradha Gupta <shradhagupta@...ux.microsoft.com>
To: Yury Norov <yury.norov@...il.com>
Cc: Michael Kelley <mhklinux@...look.com>, Dexuan Cui <decui@...rosoft.com>,
	Wei Liu <wei.liu@...nel.org>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Konstantin Taranov <kotaranov@...rosoft.com>,
	Simon Horman <horms@...nel.org>, Leon Romanovsky <leon@...nel.org>,
	Maxim Levitsky <mlevitsk@...hat.com>,
	Erni Sri Satya Vennela <ernis@...ux.microsoft.com>,
	Peter Zijlstra <peterz@...radead.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Nipun Gupta <nipun.gupta@....com>, Jason Gunthorpe <jgg@...pe.ca>,
	Jonathan Cameron <Jonathan.Cameron@...ei.com>,
	Anna-Maria Behnsen <anna-maria@...utronix.de>,
	Kevin Tian <kevin.tian@...el.com>, Long Li <longli@...rosoft.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Rob Herring <robh@...nel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczy???~Dski <kw@...ux.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	Paul Rosswurm <paulros@...rosoft.com>,
	Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH v3 4/4] net: mana: Allocate MSI-X vectors dynamically

On Wed, May 14, 2025 at 01:07:47PM -0400, Yury Norov wrote:
> On Wed, May 14, 2025 at 05:04:03AM +0000, Michael Kelley wrote:
> > From: Shradha Gupta <shradhagupta@...ux.microsoft.com> Sent: Friday, May 9, 2025 3:14 AM
> > > 
> > > Currently, the MANA driver allocates MSI-X vectors statically based on
> > > MANA_MAX_NUM_QUEUES and num_online_cpus() values and in some cases ends
> > > up allocating more vectors than it needs. This is because, by this time
> > > we do not have a HW channel and do not know how many IRQs should be
> > > allocated.
> > > 
> > > To avoid this, we allocate 1 MSI-X vector during the creation of HWC and
> > > after getting the value supported by hardware, dynamically add the
> > > remaining MSI-X vectors.
> > 
> > After this patch is applied, there are two functions for setting up IRQs:
> > 1. mana_gd_setup_dyn_irqs()
> > 2. mana_gd_setup_irqs()
> > 
> > #1 is about 78 lines of code and comments, while #2 is about 103 lines of
> > code and comments. But the two functions have a lot of commonality,
> > and that amount of commonality raises a red flag for me.
> > 
> > Have you looked at parameterizing things so a single function can serve
> > both purposes? I haven't worked through all the details, but at first
> > glance it looks very feasible, and without introducing unreasonable
> > messiness. Saving 70 to 80 lines of fairly duplicative code is worth a bit
> > of effort.
> > 
> > I have some other comments on the code. But if those two functions can
> > be combined, I'd rather re-review the result before adding comments that
> > may become irrelevant due to the restructuring.
> > 
> > Michael
> 
> On previous iteration I already mentioned that this patch is too big,
> doesn't do exactly one thing and should be split to become a reviewable 
> change. Shradha split-out the irq_setup() piece, and your review proves
> that splitting helps to reviewability.
> 
> The rest of the change is still a mess. I agree that the functions you
> mention likely duplicate each other. But overall, this patch bomb is
> above my ability to review. Fortunately, I don't have to.
> 
> Thanks,
> Yury

Hi Michael, Yury,

My intentions for keeping all these changes in MANA initialization code
together in a patch were to avoid multiple patches touching same
functions throughout the patchset. I felt it would become error-prone and
hamper reviewability.

About having to combine mana_gd_setup_dyn_irqs() and
mana_gd_setup_irqs(), we initially went ahead with a single function for
this with multiple parameters to prevent code duplication. But it made the
function very hard to read and caused a lot of confusion while reviewing
it.

At the surface of it, it would like like just two conditions to be
dealth with in the function but it actually has to deal with many more
conditions, like -
1. static allocation of all IRQs (IRQs < num_vCPUs)
2. static allocation of all IRQs (IRQs > num_vCPUS)
3. static allocation of 1 HWC IRQ
4. dynamic allocation of remaining IRQs (IRQs < num_vCPUs)
5. dynamic allocation of remaining IRQs (IRQs > num_vCPUs)

Therefore, to keep the code more readable we decided to separate the
functaionalities.

Thanks,
Shradha.

Shradha.