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

[Zhang Boyang]

Hi,

On 2022/9/12 23:03, Heinrich Schuchardt wrote:
> 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().

grub_mm_add_region_fn() can deal with page alignment, so rsize needn't 
to be page aligned.

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

There are costs for region management. I didn't investigated the 
details, but I think 1MiB is enough. The alignment is also another 
consideration, it also need extra padding.

I think I should probably set "rsize" to "size + align + 0x100000". 
Patch V2 on the way...


> 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;
> >       }