Re: [UNIKRAFT PATCH 0/2] plat/{kvm, linuxu}: statically allocate IRQ handlers

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Costin,

thanks for your comments; answers inline.

On Tue, 2020-06-23 at 12:48 +0300, Costin Lupu wrote:
> [...]
> Although these changes are quite useful, I do have the following
> comments:
> 
> 1) It's not uncommon to allocate memory for IRQ handling. Linux does
> it.
> But this is a more generic problem for which we don't have a solution
> upstream: providing memory allocation means for any kernel subsystem,
> meaning that some memory allocator should be available for the IRQ
> subsystem as well.

Yes, Linux has this boot-time allocator which is available early in the
boot process. We might have something like that in the future.

> 2) As I said, Linux does it. But it's fine if we want to have the IRQ
> handling data preallocated. What's not fine is replacing one
> functionality with another. If we will find that we also need the
> dynamic registering for some research idea we will have to revert to
> the
> old changes. What I'm trying to say is that it would be better to
> have
> an abstraction that would provide either dynamically or statically
> allocated IRQ handling data.

My understanding is that we can assume in the majority of cases that
the maximum number of IRQ handlers per line is known; unikernels are
typically compiled on purpose for a given setup. Avoiding the reliance
on a general-purpose allocator in the IRQ subsystem seems to me like a
good practice with regard to specialization and performance.

Now, I understand your point, but I am not sure that adding complexity
to the code with another abstraction will make things much better.
Also, I do not have concrete examples of a practical system where
dynamically allocated handlers would be required, but that might be due
to my lack of hindsight on this matter.

> 3) Speaking of abstractions... What happened with the changes for
> Xen?
> Isn't Xen a target platform anymore? Besides this, your changes add
> duplicated code. The IRQ handling could have been changed/refactored
> even more so that we could use the same code for all platforms. If we
> want to touch these areas now then we should do it properly.
> Otherwise
> the code will divert *again* towards opposite directions for each
> platform.

Xen does not make use of the allocator for the IRQ subsystem. I am not
very familiar with this code base, but it seems that it relies on
static allocation.

Doing a large refactoring of the platform code is not something I can
do at the moment, from a time perspective. If this is necessary, this
patch will be put on hold.

> 4) I see you're replacing __MAX_IRQ with specialized values for each
> platform. What's the reason behind that? Btw, __MAX_IRQ was
> introduced
> for the ARM changes. Did you tested this for ARM as well?

I might be mistaken, but my intention is not to replace __MAX_IRQ. I
did not modify this part of the logic. I am just introducing config
variables to predefine the number of handlers per interrupt line.

Yes, this code will be reviewed with ARM in mind. :-)

cheers,
Hugo