Re: [PATCH v3 0/6] Runtime allocation of memory regions

[Daniel Axtens]

Daniel Kiper <dkiper@net-space.pl> writes:

> Hey,
>
> Adding Daniel Axtens...
>
> On Sun, Aug 15, 2021 at 01:09:06PM +0200, Patrick Steinhardt wrote:
>> Hi,
>>
>> this is the third version of my patch set to implement runtime
>> allocation of additional memory regions.
>>
>> Changes compared to v2:
>>
>>     - A new preparatory patch was added to remove unused code which
>>       unloaded modules on OOM.
>>
>>     - Patch 2/4 has been split up into two patches: one to drop the
>>       logic where we request a quarter of available memory and then
>>       put bounds to it, and one to split apart request of additional
>>       regions and initialization of the EFI MM system.
>>
>>     - Flags are now `unsigned int` instead of `unsigned`.
>>
>>     - `add_memory_regions ()` now gets all flags instead of only a
>>       single flag `consecutive`.
>>
>>     - Flags are now defines and not an enum anymore.
>>
>>     - The callback function is now called `grub_mm_add_region_func_t`
>>       instead of `grub_mm_region_func_t`. Flags and its variable have
>>       been renamed accordingly.
>>
>> Patrick
>>
>> Patrick Steinhardt (6):
>>   mm: Drop unused unloading of modules on OOM
>>   mm: Allow dynamically requesting additional memory regions
>>   efi: mm: Always request a fixed number of pages on init
>>   efi: mm: Extract function to add memory regions
>>   efi: mm: Pass up errors from `add_memory_regions ()`
>>   efi: mm: Implement runtime addition of pages
>>
>>  grub-core/kern/dl.c     | 20 ----------
>>  grub-core/kern/efi/mm.c | 83 +++++++++++++++++++----------------------
>>  grub-core/kern/mm.c     | 12 +++---
>>  include/grub/dl.h       |  1 -
>>  include/grub/mm.h       | 16 ++++++++
>>  5 files changed, 61 insertions(+), 71 deletions(-)
>
> Patrick, I went quickly through this patch series and in general it
> LGTM. There are some minor issues but we can fix them later. Thank
> you for doing this work.
>
> Stefan and/or Daniel Axtens, may I ask you to test these patches with
> your use case? If it works for you please repost this patch series with
> your changes added. Then I will merge it after final review.

Sure, I'll have a look.

My initial thoughts are:

 - with the CAS support patch. We would still need that, and would want
   to do that call early as possible because it will cause the partition
   to be rebooted.

 - We have to be careful where we ask for memory because the kernel
   assumes that there will be some free memory below a particular address.

  - I'd also want to verify what the performance impact would be - not
    just on powerpc-ieee1275 but also on efi - of going out to
    OpenFirmware/UEFI for each new zone...

I'll test this early next week and report back.

Kind regards,
Daniel

>
> Daniel