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...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
