Re: [PATCH 00/46] Dynamic allocation of reserved_mem array.

[Rob Herring <robh@kernel.org>]

On Thu, Feb 01, 2024 at 09:08:06AM -0800, Oreoluwa Babatunde wrote:
> 
> On 1/30/2024 4:07 PM, Rob Herring wrote:
> > On Fri, Jan 26, 2024 at 03:53:39PM -0800, Oreoluwa Babatunde wrote:
> >> The reserved_mem array is used to store data for the different
> >> reserved memory regions defined in the DT of a device.  The array
> >> stores information such as region name, node, start-address, and size
> >> of the reserved memory regions.
> >>
> >> The array is currently statically allocated with a size of
> >> MAX_RESERVED_REGIONS(64). This means that any system that specifies a
> >> number of reserved memory regions greater than MAX_RESERVED_REGIONS(64)
> >> will not have enough space to store the information for all the regions.
> >>
> >> Therefore, this series extends the use of the static array for
> >> reserved_mem, and introduces a dynamically allocated array using
> >> memblock_alloc() based on the number of reserved memory regions
> >> specified in the DT.
> >>
> >> Some architectures such as arm64 require the page tables to be setup
> >> before memblock allocated memory is writable.  Therefore, the dynamic
> >> allocation of the reserved_mem array will need to be done after the
> >> page tables have been setup on these architectures. In most cases that
> >> will be after paging_init().
> >>
> >> Reserved memory regions can be divided into 2 groups.
> >> i) Statically-placed reserved memory regions
> >> i.e. regions defined in the DT using the @reg property.
> >> ii) Dynamically-placed reserved memory regions.
> >> i.e. regions specified in the DT using the @alloc_ranges
> >>     and @size properties.
> >>
> >> It is possible to call memblock_reserve() and memblock_mark_nomap() on
> >> the statically-placed reserved memory regions and not need to save them
> >> to the reserved_mem array until memory is allocated for it using
> >> memblock, which will be after the page tables have been setup.
> >> For the dynamically-placed reserved memory regions, it is not possible
> >> to wait to store its information because the starting address is
> >> allocated only at run time, and hence they need to be stored somewhere
> >> after they are allocated.
> >> Waiting until after the page tables have been setup to allocate memory
> >> for the dynamically-placed regions is also not an option because the
> >> allocations will come from memory that have already been added to the
> >> page tables, which is not good for memory that is supposed to be
> >> reserved and/or marked as nomap.
> >>
> >> Therefore, this series splits up the processing of the reserved memory
> >> regions into two stages, of which the first stage is carried out by
> >> early_init_fdt_scan_reserved_mem() and the second is carried out by
> >> fdt_init_reserved_mem().
> >>
> >> The early_init_fdt_scan_reserved_mem(), which is called before the page
> >> tables are setup is used to:
> >> 1. Call memblock_reserve() and memblock_mark_nomap() on all the
> >>    statically-placed reserved memory regions as needed.
> >> 2. Allocate memory from memblock for the dynamically-placed reserved
> >>    memory regions and store them in the static array for reserved_mem.
> >>    memblock_reserve() and memblock_mark_nomap() are also called as
> >>    needed on all the memory allocated for the dynamically-placed
> >>    regions.
> >> 3. Count the total number of reserved memory regions found in the DT.
> >>
> >> fdt_init_reserved_mem(), which should be called after the page tables
> >> have been setup, is used to carry out the following:
> >> 1. Allocate memory for the reserved_mem array based on the number of
> >>    reserved memory regions counted as mentioned above.
> >> 2. Copy all the information for the dynamically-placed reserved memory
> >>    regions from the static array into the new allocated memory for the
> >>    reserved_mem array.
> >> 3. Add the information for the statically-placed reserved memory into
> >>    reserved_mem array.
> >> 4. Run the region specific init functions for each of the reserve memory
> >>    regions saved in the reserved_mem array.
> > I don't see the need for fdt_init_reserved_mem() to be explicitly called 
> > by arch code. I said this already, but that can be done at the same time 
> > as unflattening the DT. The same conditions are needed for both: we need 
> > to be able to allocate memory from memblock.
> >
> > To put it another way, if fdt_init_reserved_mem() can be called "early", 
> > then unflattening could be moved earlier as well. Though I don't think 
> > we should optimize that. I'd rather see all arches call the DT functions 
> > at the same stages.
> Hi Rob,
> 
> The reason we moved fdt_init_reserved_mem() back into the arch specific code
> was because we realized that there was no apparently obvious way to call
> early_init_fdt_scan_reserved_mem() and fdt_init_reserved_mem() in the correct
> order that will work for all archs if we placed fdt_init_reserved_mem() inside the
> unflatten_devicetree() function.
> 
> early_init_fdt_scan_reserved_mem() needs to be
> called first before fdt_init_reserved_mem(). But on some archs,
> unflatten_devicetree() is called before early_init_fdt_scan_reserved_mem(), which
> means that if we have fdt_init_reserved_mem() inside the unflatten_devicetree()
> function, it will be called before early_init_fdt_scan_reserved_mem().
> 
> This is connected to your other comments on Patch 7 & Patch 14.
> I agree, unflatten_devicetree() should NOT be getting called before we reserve
> memory for the reserved memory regions because that could cause memory to be
> allocated from regions that should be reserved.
> 
> Hence, resolving this issue should allow us to call fdt_init_reserved_mem() from
> the  unflatten_devicetree() function without it changing the order that we are
> trying to have.

There's one issue I've found which is unflatten_device_tree() isn't 
called for ACPI case on arm64. Turns out we need /reserved-memory 
handled in that case too. However, I think we're going to change 
calling unflatten_device_tree() unconditionally for another reason[1]. 

[1] https://lore.kernel.org/all/efe6a7886c3491cc9c225a903efa2b1e.sboyd@kernel.org/

> 
> I will work on implementing this and send another revision.

I think we should go with a simpler route that's just copy the an 
initial array in initdata to a properly sized, allocated array like the 
patch below. Of course it will need some arch fixes and a follow-on 
patch to increase the initial array size.

8<--------------------------------------------------------------------
From: Rob Herring <robh@kernel.org>
Date: Wed, 31 Jan 2024 16:26:23 -0600
Subject: [PATCH] of: reserved-mem: Re-allocate reserved_mem array to actual
 size

In preparation to increase the static reserved_mem array size yet again,
copy the initial array to an allocated array sized based on the actual
size needed. Now increasing the the size of the static reserved_mem
array only eats up the initdata space. For platforms with reasonable
number of reserved regions, we have a net gain in free memory.

In order to do memblock allocations, fdt_init_reserved_mem() is moved a
bit later to unflatten_device_tree(). On some arches this is effectively
a nop.

Signed-off-by: Rob Herring <robh@kernel.org>
---
RFC as this is compile tested only. This is an alternative to this
series[1].

[1] https://lore.kernel.org/all/20240126235425.12233-1-quic_obabatun@quicinc.com/
---
 drivers/of/fdt.c             |  4 ++--
 drivers/of/of_reserved_mem.c | 18 +++++++++++++-----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bf502ba8da95..14360f5191ae 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -645,8 +645,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
 			break;
 		memblock_reserve(base, size);
 	}
-
-	fdt_init_reserved_mem();
 }
 
 /**
@@ -1328,6 +1326,8 @@ bool __init early_init_dt_scan(void *params)
  */
 void __init unflatten_device_tree(void)
 {
+	fdt_init_reserved_mem();
+
 	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
 				early_init_dt_alloc_memory_arch, false);
 
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 7ec94cfcbddb..ae323d6b25ad 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -27,7 +27,8 @@
 #include "of_private.h"
 
 #define MAX_RESERVED_REGIONS	64
-static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
+static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS] __initdata;
+static struct reserved_mem *reserved_mem_p;
 static int reserved_mem_count;
 
 static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
@@ -354,6 +355,13 @@ void __init fdt_init_reserved_mem(void)
 			}
 		}
 	}
+
+	reserved_mem_p = memblock_alloc(sizeof(struct reserved_mem) * reserved_mem_count,
+					sizeof(struct reserved_mem));
+	if (WARN(!reserved_mem_p, "of: reserved-memory allocation failed, continuing with __initdata array!\n"))
+		reserved_mem_p = reserved_mem;
+	else
+		memcpy(reserved_mem_p, reserved_mem, sizeof(struct reserved_mem) * reserved_mem_count);
 }
 
 static inline struct reserved_mem *__find_rmem(struct device_node *node)
@@ -364,8 +372,8 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node)
 		return NULL;
 
 	for (i = 0; i < reserved_mem_count; i++)
-		if (reserved_mem[i].phandle == node->phandle)
-			return &reserved_mem[i];
+		if (reserved_mem_p[i].phandle == node->phandle)
+			return &reserved_mem_p[i];
 	return NULL;
 }
 
@@ -507,8 +515,8 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np)
 
 	name = kbasename(np->full_name);
 	for (i = 0; i < reserved_mem_count; i++)
-		if (!strcmp(reserved_mem[i].name, name))
-			return &reserved_mem[i];
+		if (!strcmp(reserved_mem_p[i].name, name))
+			return &reserved_mem_p[i];
 
 	return NULL;
 }
-- 
2.43.0