Patrick Steinhardt <p...@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 <p...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
