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

Shradha Gupta
2025-05-15T05:10:55Z

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@linux.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.