Re: [PATCH v3 2/6] mm: Allow dynamically requesting additional memory regions

[Daniel Axtens]

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

> On Thu, Sep 02, 2021 at 12:48:24AM +1000, Daniel Axtens wrote:
>> 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!
>
> I think at "firmware memory allocation" level we could always allocate
> page aligned regions. Of course this may not satisfy allocations
> aligned at larger values than a page. Though I think we can solve this
> by allocating larger regions from firmware. Please look below for more
> details...
>
>>  - 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.
>
> I think we could allocate at least e.g. 128 MiB from firmware if there is
> not enough memory available in the GRUB mm. This way we would avoid frequent
> calls to firmware and could satisfy requests for larger alignments.
>
>>  - 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.
>
> Could you try the solution proposed above? Maybe it will solve problem of
> frequent additions of memory to the GRUB mm.
>
>>  - 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.
>
> Yeah, this is similar to what I proposed above. Though I would want to see
> larger numbers tested as I said earlier.
>
>>  - 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.
>
> Ugh... This can be difficult. I am not sure the GRUB mm is smart enough
> to release memory regions if they are not used anymore by it.

I didn't explain this well. What I'm trying to say is this:

Say you require 1GB of memory temporarily.

You do this:

  {
    void *mem = grub_large_malloc(1024 * 1024 * 1024);
    operate_upon(mem);
    grub_large_free(mem);
  }

large_malloc and large_free go directly to firmware. We bypass the
existing grub mm infrastructure completely. This way, people who need
this sort of very large allocation do it in a way that can return the
memory to firmware.

The reason I like this approach more than integrating the memory into
the grub mm infrastructure is that it would be very possible to get
yourself into an unbootable situation by requesting so much memory from
firmware that there's no memory left to load the kernel and initrd into.
In the case of powerpc-ieee1275 there's another failure mode where can
load the kernel but then the kernel can't claim memory from FW for it's
purposes (this is before it quiesces openfirmware and takes control of
memory management itself) and so fails to boot.

I'll try your suggestions around 128MB allocations next week - I think
they will mostly work but I'm just a bit worried about the implications of
letting grub take arbitrarily large chunks of memory.

Kind regards,
Daniel

>
>> 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
>
> I am not very happy with allocating 1/4 of memory at start of the day.
> I think allocating larger chunks of memory from firmware should be
> enough to make things working as expected.
>
>> of memory that are directly allocated from, and directly returned to,
>> the firmware.
>
> Daniel