Re: [PATCH v2 1/1] mm: Better handling of adding new regions

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]

On Tue, Sep 13, 2022 at 01:49:52AM +0800, Zhang Boyang wrote:
> The code of dynamically adding new regions has two problems. First, it
> always invalidate disk caches, which decreases performance severely.
> Second, it request exactly "size" bytes for new region, ignoring region
> management overheads.
> 
> This patch makes adding new regions more priority than disk cache
> invalidating. This patch also use "size + align + GRUB_MM_HEAP_GROW" as
> the size of new region. GRUB_MM_HEAP_GROW is set to 1MiB. This value can
> address the region overheads, and it can also improve the performance of
> small allocations when default heap is full.
> 
> Fixes: 887f98f0db43 (mm: Allow dynamically requesting additional memory regions)

It might be sensible to split this up into two patches, one to change
when we drop caches and one to round requested sizes more intelligently.

> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> ---
>  grub-core/kern/mm.c | 27 +++++++++++++++------------
>  include/grub/mm.h   |  2 ++
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index 75f6eacbe..0836b9538 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -410,18 +410,21 @@ grub_memalign (grub_size_t align, grub_size_t size)
>  {
>    grub_mm_region_t r;
>    grub_size_t n = ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) + 1;
> +  grub_size_t grow;
>    int count = 0;
>  
>    if (!grub_mm_base)
>      goto fail;
>  
> -  if (size > ~(grub_size_t) align)
> +  if (size > ~(grub_size_t) align ||
> +      (size + align) > ~(grub_size_t) GRUB_MM_HEAP_GROW)
>      goto fail;
>  
>    /* We currently assume at least a 32-bit grub_size_t,
> -     so limiting allocations to <adress space size> - 1MiB
> +     so limiting heap growth to <adress space size> - 1MiB
>       in name of sanity is beneficial. */
> -  if ((size + align) > ~(grub_size_t) 0x100000)
> +  grow = size + align + GRUB_MM_HEAP_GROW;
> +  if (grow > ~(grub_size_t) 0x100000)
>      goto fail;

I wonder whether we want to be a bit more intelligent. It feels like the
wrong thing to do to always add 1MB to the request regardless of the
requested size. It is probably sensible for small requests, but when you
request hundreds of megabytes adding a single megabyte feels rather
worthless to me.

Maybe we could use some kind of buckets instead, e.g.:

    - Up to 256kB: allocate 1MB.
    - Up to 2048kB: allocate 8MB.
    - Up to 16MB: allocate 64MB.

I just make up these numbers, but they should help demonstrate what I
mean.

>    align = (align >> GRUB_MM_ALIGN_LOG2);
> @@ -443,22 +446,16 @@ grub_memalign (grub_size_t align, grub_size_t size)
>    switch (count)
>      {
>      case 0:
> -      /* Invalidate disk caches.  */
> -      grub_disk_cache_invalidate_all ();
> -      count++;
> -      goto again;
> -
> -    case 1:

It feels sensible to reverse the order here so that we end up trying to
satisfy allocations by requesting new pages first. So only when we get
into the situation where we really cannot satisfy the request we try to
reclaim memory as a last-effort strategy.

Patrick

>        /* Request additional pages, contiguous */
>        count++;
>  
>        if (grub_mm_add_region_fn != NULL &&
> -          grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
> +          grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
>  	goto again;
>  
>        /* fallthrough  */
>  
> -    case 2:
> +    case 1:
>        /* Request additional pages, anything at all */
>        count++;
>  
> @@ -468,12 +465,18 @@ grub_memalign (grub_size_t align, grub_size_t size)
>             * Try again even if this fails, in case it was able to partially
>             * satisfy the request
>             */
> -          grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_NONE);
> +          grub_mm_add_region_fn (grow, GRUB_MM_ADD_REGION_NONE);
>            goto again;
>          }
>  
>        /* fallthrough */
> +    case 2:
> +      /* Invalidate disk caches.  */
> +      grub_disk_cache_invalidate_all ();
> +      count++;
> +      goto again;
> +
>      default:
>        break;
>      }
> diff --git a/include/grub/mm.h b/include/grub/mm.h
> index f3bf87fa0..463096cd8 100644
> --- a/include/grub/mm.h
> +++ b/include/grub/mm.h
> @@ -29,6 +29,8 @@
>  # define NULL	((void *) 0)
>  #endif
>  
> +#define GRUB_MM_HEAP_GROW 0x100000
> +
>  #define GRUB_MM_ADD_REGION_NONE        0
>  #define GRUB_MM_ADD_REGION_CONSECUTIVE (1 << 0)
>  
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]