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:

quoted

From: Shradha Gupta <redacted> Sent: Friday, May 9, 2025 3:14 AM

quoted

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.