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

[Daniel Kiper]

On Fri, Aug 27, 2021 at 01:39:05PM +1000, Daniel Axtens wrote:
> 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...

Good point! I was thinking about performance at some point too. Sadly
I forgot to say something about that... Patrick, did you compare
performance before and after you patch series? If the impact is
significant maybe we should not change initial allocation strategy
and add a function to allocate more memory during runtime only...

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

Cool! Thanks a lot!

Daniel