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

[Heinrich Schuchardt]

On 9/12/22 16:19, 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
> overheads.
>
> This patch makes adding new regions more priority than disk cache
> invalidating. This patch also use "size + 1MB" as the size of new
> region. This value can address the region overheads, and it can also
> improve performance of small allocations when default heap is full.
>
> Fixes: 887f98f0db43 (mm: Allow dynamically requesting additional memory regions)
>
> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> ---
>   grub-core/kern/mm.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index 75f6eacbe..e998454f8 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -410,6 +410,7 @@ 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 rsize;
>     int count = 0;
>   
>     if (!grub_mm_base)
> @@ -423,6 +424,7 @@ grub_memalign (grub_size_t align, grub_size_t size)
>        in name of sanity is beneficial. */
>     if ((size + align) > ~(grub_size_t) 0x100000)
>       goto fail;
> +  rsize = size + 0x100000;

rsize should be multiple of the EFI page size (4 KiB) as we call 
AllocatPages().

For size << 1 MiB I see the point.
But does it make sense to allocate another 1 MiB if size >= 1 MiB?

Best regards

Heinrich

>   
>     align = (align >> GRUB_MM_ALIGN_LOG2);
>     if (align == 0)
> @@ -443,22 +445,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:
>         /* 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 (rsize, GRUB_MM_ADD_REGION_CONSECUTIVE) == 
> GRUB_ERR_NONE)
>         goto again;
>   
>         /* fallthrough  */
>   
> -    case 2:
> +    case 1:
>         /* Request additional pages, anything at all */
>         count++;
>   
> @@ -468,12 +464,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 (rsize, GRUB_MM_ADD_REGION_NONE);
>             goto again;
>           }
>   
>         /* fallthrough */
>   
> +    case 2:
> +      /* Invalidate disk caches.  */
> +      grub_disk_cache_invalidate_all ();
> +      count++;
> +      goto again;
> +
>       default:
>         break;
>       }