[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/6] mm: Allow dynamically requesting additional memory re
From: |
Daniel Axtens |
Subject: |
Re: [PATCH v3 2/6] mm: Allow dynamically requesting additional memory regions |
Date: |
Thu, 02 Sep 2021 00:48:24 +1000 |
Patrick Steinhardt <ps@pks.im> writes:
> Currently, all platforms will set up their heap on initialization of the
> platform code. While this works mostly fine, it poses some limitations
> on memory management on us. Most notably, allocating big chunks of
> memory in the gigabyte range would require us to pre-request this many
> bytes from the firmware and add it to the heap from the beginning on
> some platforms like EFI. As this isn't needed for most configurations,
> it is inefficient and may even negatively impact some usecases when,
> e.g., chainloading. Nonetheless, allocating big chunks of memory is
> required sometimes, where one example is the upcoming support for the
> Argon2 key derival function in LUKS2.
>
> In order to avoid pre-allocating big chunks of memory, this commit
> implements a runtime mechanism to add more pages to the system. When a
> given allocation cannot be currently satisfied, we'll call a given
> callback set up by the platform's own memory management subsystem,
> asking it to add a memory area with at least `n` bytes. If this
> succeeds, we retry searching for a valid memory region, which should now
> succeed.
>
I implemented this for ieee1275-powerpc. I set the initial memory claim
to 1MB to match EFI and to exercise the code.
Thoughts as I progressed:
- You probably need to think about how to satisfy requests with
particular alignments: currently there is no way to specify that with
the current interface, and I saw powerpc-ieee1275 return bunch of
allocations at e.g 0x2a561e which is not particularly well aligned!
- You haven't included in the calculations the extra space required for
mm housekeeping. For example, I'm seeing an allocation for 32kB be
requested via grub_mm_add_region_fn. I dutifully allocate 32kB, with
no alignment requirements, and pass that pointer to
grub_mm_init_region. grub_mm_init_region throws away bytes at the
start to get to GRUB_MM_ALIGN, then uses some bytes for the
grub_mm_header_t, then any actual allocation uses bytes for the
malloc metadata. So the actual underlying allocation cannot be
satisfied.
I think you get away with this on EFI because you use BYTES_TO_PAGES
and get page-aligned memory, but I think you should probably round up
to the next power of 2 for smaller allocations or to the next page or
so for larger allocations.
- After fixing that in the ieee1275 code, all_functional_test
hangs trying to run the cmdline_cat test. I think this is from a slow
algorithm somewhere - the grub allocator isn't exactly optimised for
a proliferation of regions.
- I noticed that nearly all the allocations were under 1MB. This seems
inefficient for a trip out to firmware. So I made the ieee1275 code
allocate at least max(4MB, (size of allocation rounded up nearest
1MB) + 4kB). This makes the tests run with only the usual failures,
at least on pseries with debug on... still chasing some bugs beyond
that.
- The speed impact depends on the allocation size. I'll post something
on that tomorrow, hopefully, but larger minimum allocations work
noticably better.
- We only have 4GB max to play with because (at least) powerpc-ieee1275
is technically defined to be 32 bit. So I'm a bit nervous about
further large allocations unless we have a way to release them back
to _firmware_, not just to grub.
I would think a better overall approach would be to allocate the 1/4 of
ram when grub starts, and create a whole new interface for large slabs
of memory that are directly allocated from, and directly returned to,
the firmware.
Kind regards,
Daniel
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> grub-core/kern/mm.c | 10 ++++++++++
> include/grub/mm.h | 16 ++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index e0e580270..2df835392 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -81,6 +81,7 @@
>
>
> grub_mm_region_t grub_mm_base;
> +grub_mm_add_region_func_t grub_mm_add_region_fn;
>
> /* Get a header from the pointer PTR, and set *P and *R to a pointer
> to the header and a pointer to its region, respectively. PTR must
> @@ -360,6 +361,15 @@ grub_memalign (grub_size_t align, grub_size_t size)
> count++;
> goto again;
>
> + case 1:
> + /* Request additional pages. */
> + count++;
> +
> + if (grub_mm_add_region_fn && grub_mm_add_region_fn (size,
> GRUB_MM_ADD_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
> + goto again;
> +
> + /* fallthrough */
> +
> default:
> break;
> }
> diff --git a/include/grub/mm.h b/include/grub/mm.h
> index 9c38dd3ca..afde57d2e 100644
> --- a/include/grub/mm.h
> +++ b/include/grub/mm.h
> @@ -20,6 +20,7 @@
> #ifndef GRUB_MM_H
> #define GRUB_MM_H 1
>
> +#include <grub/err.h>
> #include <grub/types.h>
> #include <grub/symbol.h>
> #include <config.h>
> @@ -28,6 +29,21 @@
> # define NULL ((void *) 0)
> #endif
>
> +#define GRUB_MM_ADD_REGION_NONE 0
> +#define GRUB_MM_ADD_REGION_CONSECUTIVE (1 << 0)
CONSECUTIVE needs a definition here. Initially I thought it meant that
the request had to expand an existing region, but what I think it means
having read your EFI implementation is that the request needs to be
satisfied by a single region rather than by a number of smaller regions.
(NONE probably also needs a better name, because it seemed a bit odd to
type ADD_REGION_NONE. Even ADD_REGION_FLAGS_NONE would be better.)
> +
> +/*
> + * Function used to request memory regions of `grub_size_t` bytes. The second
> + * parameter is a bitfield of `GRUB_MM_ADD_REGION` flags.
> + */
> +typedef grub_err_t (*grub_mm_add_region_func_t) (grub_size_t, unsigned int);
> +
> +/*
> + * Set this function pointer to enable adding memory-regions at runtime in
> case
> + * a memory allocation cannot be satisfied with existing regions.
> + */
> +extern grub_mm_add_region_func_t EXPORT_VAR(grub_mm_add_region_fn);
> +
> void grub_mm_init_region (void *addr, grub_size_t size);
> void *EXPORT_FUNC(grub_calloc) (grub_size_t nmemb, grub_size_t size);
> void *EXPORT_FUNC(grub_malloc) (grub_size_t size);
> --
> 2.32.0
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
- Re: [PATCH v3 2/6] mm: Allow dynamically requesting additional memory regions,
Daniel Axtens <=