Re: [PATCH 2/2] mm: Preallocate some space when adding new regions

[Daniel Kiper]

On Tue, Nov 29, 2022 at 10:17:34PM +0800, Zhang Boyang wrote:
> When grub_memalign() encounters out-of-memory, it will try
> grub_mm_add_region_fn() to request more memory from system firmware.
> However, it doesn't preallocate memory space for future allocation
> requests. In extreme cases, it requires one call to
> grub_mm_add_region_fn() for each memory allocation request, which may be
> too slow if those requests are small (e.g. about 4KB).

s/, which may ... are small (e.g. about 4KB)./. This can be very slow./

> This patch introduces GRUB_MM_HEAP_GROW, the minimal heap growth

s/GRUB_MM_HEAP_GROW/GRUB_MM_HEAP_GROW_EXTRA/

> granularity. The new region size is now set to the bigger one of
> original value and GRUB_MM_HEAP_GROW. Thus, it will result in some
> memory space preallocated if current allocations request is small.
>
> The value of GRUB_MM_HEAP_GROW is set to 1MB. If this value is smaller,
> the cost of small memory allocations will be higher. If this value is
> larger, more memory will be wasted and it might cause out-of-memory on
> machines with small amount of RAM.
>
> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> ---
>  grub-core/kern/mm.c       | 3 +++
>  include/grub/mm_private.h | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index 973cb6b15..83c618c5b 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -452,6 +452,9 @@ grub_memalign (grub_size_t align, grub_size_t size)
>        if (grub_add (bound, GRUB_MM_MAX_COST, &grow))
>       goto fail;
>
> +      /* Calculate optimal size of heap growth. */
> +      grow = grub_max (grow, GRUB_MM_HEAP_GROW);

This should be "if (grub_add (grow, GRUB_MM_HEAP_GROW, &grow))..."

And it should happen before first grub_mm_add_region_fn() call.

>        /* Request additional pages, contiguous */
>        count++;
>
> diff --git a/include/grub/mm_private.h b/include/grub/mm_private.h
> index f212110e4..268fbf094 100644
> --- a/include/grub/mm_private.h
> +++ b/include/grub/mm_private.h
> @@ -104,6 +104,9 @@ typedef struct grub_mm_region
>   */
>  #define GRUB_MM_MAX_COST 0x1000
>
> +/* Minimal heap growth granularity when existing heap space is exhausted. */
> +#define GRUB_MM_HEAP_GROW 0x100000

Is 1 MiB enough? I think yes but prefer to double check...

Daniel