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

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


Best Regards,
Zhang Boyang 

[PATCH 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
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;
 
   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;
     }
-- 
2.30.2

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

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