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

[Patrick Steinhardt]

On Mon, Aug 30, 2021 at 07:49:07PM +0200, Daniel Kiper wrote:
> 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

The question is how to measure performance. Are there any benchmarks
that result in somewhat reproducible results?

Patrick

signature.asc
Description: PGP signature