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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi Hugo,

Please see inline.

On 6/24/20 10:53 AM, Hugo Lefeuvre wrote:
> 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.
> 

The refactoring would target only the IRQ subsystem. Your current
changes remove the need for an allocator to be passed to init functions
and this affects the interface/API. This means that we can get rid of
the allocator if we keep only the static allocation and it would have to
be done for all platforms. Anyway, the current version of changes leave
the design in a fishy state.

>> 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. :-)

Sorry about that, I misread the patch and I wasn't clear enough in my
statement. What I meant is that there was no need for specialization
before (in case of __MAX_IRQ we had one single value). So why are your
changes introducing different values for the handlers numbers? What's
the gain here? Isn't that adding more complexity to the configuration
space? Why didn't you use only one config option for both platforms? It
looks like we have an inconsistency here between __MAX_IRQ and the
handlers config values.

Cheers,
Costin