[PATCH v3 0/2] use large ZoneHigh when there is enough memory

[Gerd Hoffmann]

v3:
 - check size instead of end address.

Gerd Hoffmann (2):
  malloc: use variable for ZoneHigh size
  malloc: use large ZoneHigh when there is enough memory

src/config.h |  3 ++-
 src/malloc.c | 18 +++++++++++-------
 2 files changed, 13 insertions(+), 8 deletions(-)

[PATCH v3 1/2] malloc: use variable for ZoneHigh size

[Gerd Hoffmann]

Use the variable highram_size instead of the BUILD_MAX_HIGHTABLE #define
for the ZoneHigh size. Initialize the new variable with the old #define,
so behavior does not change.

This allows to easily adjust the ZoneHigh size at runtime in a followup
patch.

Signed-off-by: Gerd Hoffmann 
---
 src/malloc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/malloc.c b/src/malloc.c
index 3733855caf2c..7fa50a85595b 100644
--- a/src/malloc.c
+++ b/src/malloc.c
@@ -422,7 +422,8 @@ malloc_preinit(void)
     e820_add(BUILD_BIOS_ADDR, BUILD_BIOS_SIZE, E820_RESERVED);

// Populate temp high ram
-    u32 highram = 0;
+    u32 highram_start = 0;
+    u32 highram_size = BUILD_MAX_HIGHTABLE;
     int i;
     for (i=e820_count-1; i>=0; i--) {
         struct e820entry *en = &e820_list[i];
@@ -432,10 +433,10 @@ malloc_preinit(void)
         if (en->type != E820_RAM || end > 0xffffffff)
             continue;
         u32 s = en->start, e = end;
-        if (!highram) {
-            u32 newe = ALIGN_DOWN(e - BUILD_MAX_HIGHTABLE, MALLOC_MIN_ALIGN);
+        if (!highram_start) {
+            u32 newe = ALIGN_DOWN(e - highram_size, MALLOC_MIN_ALIGN);
             if (newe <= e && newe >= s) {
-                highram = newe;
+                highram_start = newe;
                 e = newe;
             }
         }
@@ -444,9 +445,10 @@ malloc_preinit(void)

// Populate regions
     alloc_add(&ZoneTmpLow, BUILD_STACK_ADDR, BUILD_EBDA_MINIMUM);
-    if (highram) {
-        alloc_add(&ZoneHigh, highram, highram + BUILD_MAX_HIGHTABLE);
-        e820_add(highram, BUILD_MAX_HIGHTABLE, E820_RESERVED);
+    if (highram_start) {
+        dprintf(3, "malloc: using %dk for ZoneHigh\n", highram_size / 1024);
+        alloc_add(&ZoneHigh, highram_start, highram_start + highram_size);
+        e820_add(highram_start, highram_size, E820_RESERVED);
     }
 }

[PATCH v3 2/2] malloc: use large ZoneHigh when there is enough memory

[Gerd Hoffmann]

In case there is enough memory installed use a large ZoneHigh.

Signed-off-by: Gerd Hoffmann 
---
 src/config.h | 3 ++-
 src/malloc.c | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/config.h b/src/config.h
index 93c8dbc2d530..9abe355b6c47 100644
--- a/src/config.h
+++ b/src/config.h
@@ -17,7 +17,8 @@
 // Maximum number of map entries in the e820 map
 #define BUILD_MAX_E820 32
 // Space to reserve in high-memory for tables
-#define BUILD_MAX_HIGHTABLE (256*1024)
+#define BUILD_MIN_HIGHTABLE (256*1024)
+#define BUILD_MAX_HIGHTABLE (16*1024*1024)
 // Largest supported externaly facing drive id
 #define BUILD_MAX_EXTDRIVE 16
 // Number of bytes the smbios may be and still live in the f-segment
diff --git a/src/malloc.c b/src/malloc.c
index 7fa50a85595b..967e000f51ba 100644
--- a/src/malloc.c
+++ b/src/malloc.c
@@ -423,7 +423,7 @@ malloc_preinit(void)

// Populate temp high ram
     u32 highram_start = 0;
-    u32 highram_size = BUILD_MAX_HIGHTABLE;
+    u32 highram_size = BUILD_MIN_HIGHTABLE;
     int i;
     for (i=e820_count-1; i>=0; i--) {
         struct e820entry *en = &e820_list[i];
@@ -434,6 +434,8 @@ malloc_preinit(void)
             continue;
         u32 s = en->start, e = end;
         if (!highram_start) {
+            if (e - s > BUILD_MAX_HIGHTABLE * 16)
+                highram_size = BUILD_MAX_HIGHTABLE;
             u32 newe = ALIGN_DOWN(e - highram_size, MALLOC_MIN_ALIGN);
             if (newe <= e && newe >= s) {
                 highram_start = newe;

[PATCH v3 2/2] malloc: use large ZoneHigh when there is enough memory

[Kevin O'Connor]

On Tue, Apr 26, 2022 at 10:56:42AM +0200, Gerd Hoffmann wrote:
> In case there is enough memory installed use a large ZoneHigh.
> Signed-off-by: Gerd Hoffmann
>
> src/config.h | 3 ++-
>  src/malloc.c | 4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/config.h b/src/config.h
> index 93c8dbc2d530..9abe355b6c47 100644
> --- a/src/config.h
> +++ b/src/config.h
> @@ -17,7 +17,8 @@
>  // Maximum number of map entries in the e820 map
>  #define BUILD_MAX_E820 32
>  // Space to reserve in high-memory for tables
> -#define BUILD_MAX_HIGHTABLE (256*1024)
> +#define BUILD_MIN_HIGHTABLE (256*1024)
> +#define BUILD_MAX_HIGHTABLE (16*1024*1024)
>  // Largest supported externaly facing drive id
>  #define BUILD_MAX_EXTDRIVE 16
>  // Number of bytes the smbios may be and still live in the f-segment
> diff --git a/src/malloc.c b/src/malloc.c
> index 7fa50a85595b..967e000f51ba 100644
> --- a/src/malloc.c
> +++ b/src/malloc.c
> @@ -423,7 +423,7 @@ malloc_preinit(void)
>  // Populate temp high ram
>  u32 highram_start = 0;
> u32 highram_size = BUILD_MAX_HIGHTABLE;
> u32 highram_size = BUILD_MIN_HIGHTABLE;
> int i;
> for (i=e820_count-1; i>=0; i--) {
>     struct e820entry *en = &e820_list[i];
>
> @@ -434,6 +434,8 @@ malloc_preinit(void)
>              continue;
>          u32 s = en->start, e = end;
>          if (!highram_start) {
>        if (e - s > BUILD_MAX_HIGHTABLE * 16)
>            highram_size = BUILD_MAX_HIGHTABLE;
>        u32 newe = ALIGN_DOWN(e - highram_size, MALLOC_MIN_ALIGN);
>        if (newe <= e && newe >= s) {
>            highram_start = newe;

Thanks, but I'm still seeing a corner case where this fails - if
ALIGN_DOWN() falls below s.  In general, it seems odd to assign
highram_size with the possability of not assigning highram_start.

Cheers,
-Kevin

[PATCH v3 2/2] malloc: use large ZoneHigh when there is enough memory

[Gerd Hoffmann]

> >      if (!highram_start) {
> >        if (e - s > BUILD_MAX_HIGHTABLE * 16)
> >            highram_size = BUILD_MAX_HIGHTABLE;
> >        u32 newe = ALIGN_DOWN(e - highram_size, MALLOC_MIN_ALIGN);
> >        if (newe <= e && newe >= s) {
> >            highram_start = newe;
>
> Thanks, but I'm still seeing a corner case where this fails - if
> ALIGN_DOWN() falls below s.

Given that the code requires the memory block being 16x the size of the
zonehigh allocation (i.e. use 16M in case the block is 256M or larger)
I fail to see how any reasonable alignment requirement can make that
fail ...

take care,
  Gerd

[PATCH v3 2/2] malloc: use large ZoneHigh when there is enough memory

[Kevin O'Connor]

On Wed, Apr 27, 2022 at 08:26:35AM +0200, Gerd Hoffmann wrote:
> > >      if (!highram_start) {
> > >        if (e - s > BUILD_MAX_HIGHTABLE * 16)
> > >            highram_size = BUILD_MAX_HIGHTABLE;
> > >        u32 newe = ALIGN_DOWN(e - highram_size, MALLOC_MIN_ALIGN);
> > >        if (newe <= e && newe >= s) {
> > >            highram_start = newe;
> >
> > Thanks, but I'm still seeing a corner case where this fails - if
> > ALIGN_DOWN() falls below s.
>
> Given that the code requires the memory block being 16x the size of the
> zonehigh allocation (i.e. use 16M in case the block is 256M or larger)
> I fail to see how any reasonable alignment requirement can make that
> fail ...

My mistake.

I do think your new v4 version of the code is easier to understand
though.

Cheers,
-Kevin

[PATCH v3 2/2] malloc: use large ZoneHigh when there is enough memory

[Gerd Hoffmann]

Hi,
> I do think your new v4 version of the code is easier to understand
> though.

Great, that was precisely the point of doing a v4 ;)

take care,
  Gerd