[PATCH v2 0/1] Fix slow loading problem introduced by 887f98f0d

[PATCH v2 0/1] Fix slow loading problem introduced by 887f98f0d

[Zhang Boyang]

Hi,

This patch should probably fix the slow loading problem introduced by
887f98f0db43 (mm: Allow dynamically requesting additional memory
regions).

Although I'm not against increasing default heap size, simply increasing
heap size may not fully fix the problem. I think the root cause is disk
caches are always invalidated when almost every malloc() after default
heap size is exhausted.

However, I havn't reproduced the problem, so I haven't tested my patch.
I would appreciate if someone can test this patch.

Changes in V2:
  Use GRUB_MM_HEAP_GROW constant instead of hardcoding 0x100000.
  Renamed "rsize" to "grow".
  Check "grow" against sanity limits.

Best Regards,
Zhang Boyang 

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

[Zhang Boyang]

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)

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;
 
   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:
       /* 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

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

[jim945]

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


12.09.2022 20:49, Zhang Boyang пишет:
> 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)
>
> 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;
>   
>     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:
>         /* 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)
>   


In my case it works great.

[-- Attachment #2: Type: text/html, Size: 4211 bytes --]

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

[Patrick Steinhardt]

[-- 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 --]

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

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

[jim945]

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


06.10.2022 16:07, 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

/* Invalidate disk caches. */ in case 0 causes a long loading time.

Script to assemble grab.efi and prefix in SFS. For testing this.

Does not appear in qemu.

The TEST_GRUB directory will contain files that need to be copied to the 
flash drive.



pwd="$PWD"
workdir=`dirname "$(readlink -e "$0")"`
tmp=$(mktemp -d /tmp/grub-memdisk-XXXXXXXXX)

modules="/lib/grub/x86_64-efi"

if [ -d "${tmp}" ] ; then rm -rf "${tmp}" ; fi
mkdir -p "${tmp}"/settings
cat << EOF >> "${tmp}"/settings/config.cfg
set root=memdisk
normal /boot/grub/memdisk.cfg
EOF

cat << EOF >> "${tmp}"/settings/memdisk.cfg
search -s -f /gp.loop
loopback memdiskgp /gp.loop
set prefix=(memdiskgp)/boot/grub
set root=memdiskgp
configfile /boot/grub/grub.cfg
EOF

cat << EOF >> "${tmp}"/settings/grub.cfg
menuentry "===TEST===" {
echo "===TEST==="
sleep 1
}
EOF

# Make memdisk 1
mkdir -p "${tmp}"/memdisk
mkdir -p "${tmp}"/memdisk/boot/grub
cp "${tmp}"/settings/memdisk.cfg "${tmp}"/memdisk/boot/grub/memdisk.cfg

cd "$tmp"/memdisk
find ./boot | cpio -o -H newc > "${tmp}"/memdisk.loop #2> /dev/null
cd "$pwd"
rm -r "${tmp}"/memdisk
# Make memdisk 1

# Make memdisk 2
mkdir "${tmp}"/prefix_img

mkdir -p "${tmp}"/prefix_img/boot/grub/x86_64-efi
echo "Copying x86_64-efi modules"
cp "${modules}"/*.mod "${tmp}"/prefix_img/boot/grub/x86_64-efi/
cp "${modules}"/*.lst "${tmp}"/prefix_img/boot/grub/x86_64-efi/
cp "${tmp}"/settings/grub.cfg "${tmp}"/prefix_img/boot/grub/grub.cfg
echo "Generate gp.loop"
mksquashfs "$tmp"/prefix_img/ "${tmp}"/gp.loop -all-root &> /dev/null
# Make memdisk 2

echo "Generate grubx64.efi"
builtin_pl="cpio exfat ext2 fat gzio iso9660 loopback lzopio newc normal 
ntfs part_gpt part_msdos probe read search tar test configfile echo xzio 
squash4 sfs memdisk"

grub-mkimage -d "${modules}" -m "${tmp}"/memdisk.loop -p "/boot/grub" -c 
"${tmp}"/settings/config.cfg -o "${tmp}"/grubx64.efi -O x86_64-efi 
$builtin_pl

echo "Copying files in ${workdir}/TEST_GRUB"
mkdir -p "${workdir}"/TEST_GRUB/EFI/BOOT
cp "${tmp}"/grubx64.efi "${workdir}"/TEST_GRUB/EFI/BOOT/BOOTX64.EFI
cp "${tmp}"/gp.loop "${workdir}"/TEST_GRUB/gp.loop

rm -rf "${tmp}"

[-- Attachment #2: Type: text/html, Size: 9603 bytes --]

Re: [PATCH v2 0/1] Fix slow loading problem introduced by 887f98f0d

[Gary Lin]

On Tue, Sep 13, 2022 at 01:49:51AM +0800, Zhang Boyang wrote:
> Hi,
> 
> This patch should probably fix the slow loading problem introduced by
> 887f98f0db43 (mm: Allow dynamically requesting additional memory
> regions).
> 
> Although I'm not against increasing default heap size, simply increasing
> heap size may not fully fix the problem. I think the root cause is disk
> caches are always invalidated when almost every malloc() after default
> heap size is exhausted.
> 
Deferring disk cache invalidation may not be a good idea because disk
cache is not mission-critical, so it's reasonable to invalidate the
caches to squeeze more memory blocks. The real question is still about
the default heap size. The memory allocation path is not cheap and it
should be avoided as much as possible. If the first allocated heap can
fulfill the most cases, then users won't be bothered by the performance
drop that much.

Gary Lin

> However, I havn't reproduced the problem, so I haven't tested my patch.
> I would appreciate if someone can test this patch.
> 
> Changes in V2:
>   Use GRUB_MM_HEAP_GROW constant instead of hardcoding 0x100000.
>   Renamed "rsize" to "grow".
>   Check "grow" against sanity limits.
> 
> Best Regards,
> Zhang Boyang 
> 
> 
> 

Re: [PATCH v2 0/1] Fix slow loading problem introduced by 887f98f0d

[Gary Lin]

On Tue, Sep 13, 2022 at 11:16:19AM +0800, Gary Lin via Grub-devel wrote:
> On Tue, Sep 13, 2022 at 01:49:51AM +0800, Zhang Boyang wrote:
> > Hi,
> > 
> > This patch should probably fix the slow loading problem introduced by
> > 887f98f0db43 (mm: Allow dynamically requesting additional memory
> > regions).
> > 
> > Although I'm not against increasing default heap size, simply increasing
> > heap size may not fully fix the problem. I think the root cause is disk
> > caches are always invalidated when almost every malloc() after default
> > heap size is exhausted.
> > 
> Deferring disk cache invalidation may not be a good idea because disk
> cache is not mission-critical, so it's reasonable to invalidate the
> caches to squeeze more memory blocks. The real question is still about
> the default heap size. The memory allocation path is not cheap and it
> should be avoided as much as possible. If the first allocated heap can
> fulfill the most cases, then users won't be bothered by the performance
> drop that much.
> 
I see your point now. Instead of keeping a minimal heap, your patch
maximizes the heap to a certain point and then invalidates disk caches
to sequeeze the last memory pages. This certainly benefits the
performance.

Gary Lin

> Gary Lin
> 
> > However, I havn't reproduced the problem, so I haven't tested my patch.
> > I would appreciate if someone can test this patch.
> > 
> > Changes in V2:
> >   Use GRUB_MM_HEAP_GROW constant instead of hardcoding 0x100000.
> >   Renamed "rsize" to "grow".
> >   Check "grow" against sanity limits.
> > 
> > Best Regards,
> > Zhang Boyang 
> > 
> > 
> > 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel