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

[Daniel Kiper]

On Sun, Sep 25, 2022 at 03:59:50PM +0200, Patrick Steinhardt wrote:
> 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.

Yes, I agree with Patrick.

> > 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.

Adding more than 1 MiB may lead to situation when we are not able to
boot machine which still has e.g. 64 MiB essentially free but allocated
for GRUB heap. So, I would stick with 1 MiB even if it is not very
efficient for larger sizes. Additionally, assuming large memory
allocations follow large allocations is not always (often?) true.
Though I would improve algorithm a bit:

grow = ALIGN_UP (size + GRUB_MM_HEAP_GROW, GRUB_MM_HEAP_GROW);

And more importantly this calculations should happen in switch below.

> >    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.

Yes, I agree.

Daniel