[Patch v2 0/3] memblock: some fix for memmap_init_reserved_pages()

[Patch v2 0/3] memblock: some fix for memmap_init_reserved_pages()

[Wei Yang]

During code review, I found we may leave some reserved region with invalid nid
when memblock_set_node() double the array. The patch set propose a fix and
test case.

Also I found memblock_set_node() accept size instead of end as second
parameter. Currently it is misused.

Patch 1: fix the second argument passed to memblock_set_node()
Patch 2: fix the invalid node id we may have in memmap_init_reserved_pages()
Patch 3: add a test case to verify it

v2: move check out side of the loop

Wei Yang (3):
  mm/memblock: pass size instead of end to memblock_set_node()
  mm/memblock: repeat setting reserved region nid if array is doubled
  memblock tests: add test for memblock_set_node

 mm/memblock.c                            |  12 ++-
 tools/testing/memblock/tests/basic_api.c | 102 +++++++++++++++++++++++
 2 files changed, 113 insertions(+), 1 deletion(-)

-- 
2.34.1

[Patch v2 1/3] mm/memblock: pass size instead of end to memblock_set_node()

[Wei Yang]

The second parameter of memblock_set_node() is size instead of end.

Since it iterates from lower address to higher address, finally the node
id is correct. But during the process, some of them are wrong.

Pass size instead of end.

Fixes: 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Mike Rapoport <rppt@kernel.org>
CC: Yajun Deng <yajun.deng@linux.dev>
CC: <stable@vger.kernel.org>
---
 mm/memblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memblock.c b/mm/memblock.c
index 64ae678cd1d1..85442f1b7f14 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2192,7 +2192,7 @@ static void __init memmap_init_reserved_pages(void)
 		if (memblock_is_nomap(region))
 			reserve_bootmem_region(start, end, nid);
 
-		memblock_set_node(start, end, &memblock.reserved, nid);
+		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
 
 	/*
-- 
2.34.1

Re: [Patch v2 1/3] mm/memblock: pass size instead of end to memblock_set_node()

[Anshuman Khandual]

On 3/18/25 12:49, Wei Yang wrote:
> The second parameter of memblock_set_node() is size instead of end.
> 
> Since it iterates from lower address to higher address, finally the node
> id is correct. But during the process, some of them are wrong.
> 
> Pass size instead of end.

Makes sense.

> 
> Fixes: 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")

The commit is correct here which had introduced the problem.

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Mike Rapoport <rppt@kernel.org>
> CC: Yajun Deng <yajun.deng@linux.dev>
> CC: <stable@vger.kernel.org>
> ---
>  mm/memblock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 64ae678cd1d1..85442f1b7f14 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2192,7 +2192,7 @@ static void __init memmap_init_reserved_pages(void)
>  		if (memblock_is_nomap(region))
>  			reserve_bootmem_region(start, end, nid);
>  
> -		memblock_set_node(start, end, &memblock.reserved, nid);
> +		memblock_set_node(start, region->size, &memblock.reserved, nid);
>  	}
>  
>  	/*

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

[Patch v2 2/3] mm/memblock: repeat setting reserved region nid if array is doubled

[Wei Yang]

Commit 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()") introduce
a way to set nid to all reserved region.

But there is a corner case it will leave some region with invalid nid.
When memblock_set_node() doubles the array of memblock.reserved, it may
lead to a new reserved region before current position. The new region
will be left with an invalid node id.

Repeat the process when detecting it.

Fixes: 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Mike Rapoport <rppt@kernel.org>
CC: Yajun Deng <yajun.deng@linux.dev>
CC: <stable@vger.kernel.org>

---
v2: move check out side of the loop
---
 mm/memblock.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index 85442f1b7f14..0bae7547d2db 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2179,11 +2179,14 @@ static void __init memmap_init_reserved_pages(void)
 	struct memblock_region *region;
 	phys_addr_t start, end;
 	int nid;
+	unsigned long max_reserved;
 
 	/*
 	 * set nid on all reserved pages and also treat struct
 	 * pages for the NOMAP regions as PageReserved
 	 */
+repeat:
+	max_reserved = memblock.reserved.max;
 	for_each_mem_region(region) {
 		nid = memblock_get_region_node(region);
 		start = region->base;
@@ -2194,6 +2197,13 @@ static void __init memmap_init_reserved_pages(void)
 
 		memblock_set_node(start, region->size, &memblock.reserved, nid);
 	}
+	/*
+	 * 'max' is changed means memblock.reserved has been doubled its
+	 * array, which may result a new reserved region before current
+	 * 'start'. Now we should repeat the procedure to set its node id.
+	 */
+	if (max_reserved != memblock.reserved.max)
+		goto repeat;
 
 	/*
 	 * initialize struct pages for reserved regions that don't have
-- 
2.34.1

Re: [Patch v2 2/3] mm/memblock: repeat setting reserved region nid if array is doubled

[Anshuman Khandual]

On 3/18/25 12:49, Wei Yang wrote:
> Commit 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()") introduce
> a way to set nid to all reserved region.
> 
> But there is a corner case it will leave some region with invalid nid.
> When memblock_set_node() doubles the array of memblock.reserved, it may
> lead to a new reserved region before current position. The new region
> will be left with an invalid node id.

But is it really possible for the memblock array to double during
memmap_init_reserved_pages() ? Just wondering - could you please
give some example scenarios.

> 
> Repeat the process when detecting it.
> 
> Fixes: 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Mike Rapoport <rppt@kernel.org>
> CC: Yajun Deng <yajun.deng@linux.dev>
> CC: <stable@vger.kernel.org>
> 
> ---
> v2: move check out side of the loop
> ---
>  mm/memblock.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 85442f1b7f14..0bae7547d2db 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2179,11 +2179,14 @@ static void __init memmap_init_reserved_pages(void)
>  	struct memblock_region *region;
>  	phys_addr_t start, end;
>  	int nid;
> +	unsigned long max_reserved;
>  
>  	/*
>  	 * set nid on all reserved pages and also treat struct
>  	 * pages for the NOMAP regions as PageReserved
>  	 */
> +repeat:
> +	max_reserved = memblock.reserved.max;
>  	for_each_mem_region(region) {
>  		nid = memblock_get_region_node(region);
>  		start = region->base;
> @@ -2194,6 +2197,13 @@ static void __init memmap_init_reserved_pages(void)
>  
>  		memblock_set_node(start, region->size, &memblock.reserved, nid);
>  	}
> +	/*
> +	 * 'max' is changed means memblock.reserved has been doubled its
> +	 * array, which may result a new reserved region before current
> +	 * 'start'. Now we should repeat the procedure to set its node id.
> +	 */
> +	if (max_reserved != memblock.reserved.max)
> +		goto repeat;
>  
>  	/*
>  	 * initialize struct pages for reserved regions that don't have

Re: [Patch v2 2/3] mm/memblock: repeat setting reserved region nid if array is doubled

[Wei Yang]

On Tue, Mar 18, 2025 at 04:25:23PM +0530, Anshuman Khandual wrote:
>On 3/18/25 12:49, Wei Yang wrote:
>> Commit 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()") introduce
>> a way to set nid to all reserved region.
>> 
>> But there is a corner case it will leave some region with invalid nid.
>> When memblock_set_node() doubles the array of memblock.reserved, it may
>> lead to a new reserved region before current position. The new region
>> will be left with an invalid node id.
>
>But is it really possible for the memblock array to double during
>memmap_init_reserved_pages() ? Just wondering - could you please
>give some example scenarios.
>

The possibility is low, but I think it is possible.

I have created a test case to reproduce it. Not sure it could explain ?

[Patch v2 3/3] memblock tests: add test for memblock_set_node

[Wei Yang]

Add a test to check memblock_set_node() behavior.

And create a corner case in which the memblock.reserved array is doubled
during memblock_set_node(). And finally make sure all regions in
memblock.reserved are with valid node id.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Mike Rapoport <rppt@kernel.org>
CC: Yajun Deng <yajun.deng@linux.dev>

---
v2: move the check out side loop
---
 tools/testing/memblock/tests/basic_api.c | 102 +++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c
index 67503089e6a0..01e836fba488 100644
--- a/tools/testing/memblock/tests/basic_api.c
+++ b/tools/testing/memblock/tests/basic_api.c
@@ -2434,6 +2434,107 @@ static int memblock_overlaps_region_checks(void)
 	return 0;
 }
 
+#ifdef CONFIG_NUMA
+static int memblock_set_node_check(void)
+{
+	unsigned long i, max_reserved;
+	struct memblock_region *rgn;
+	void *orig_region;
+
+	PREFIX_PUSH();
+
+	reset_memblock_regions();
+	memblock_allow_resize();
+
+	dummy_physical_memory_init();
+	memblock_add(dummy_physical_memory_base(), MEM_SIZE);
+	orig_region = memblock.reserved.regions;
+
+	/* Equally Split range to node 0 and 1*/
+	memblock_set_node(memblock_start_of_DRAM(),
+			memblock_phys_mem_size() / 2, &memblock.memory, 0);
+	memblock_set_node(memblock_start_of_DRAM() + memblock_phys_mem_size() / 2,
+			memblock_phys_mem_size() / 2, &memblock.memory, 1);
+
+	ASSERT_EQ(memblock.memory.cnt, 2);
+	rgn = &memblock.memory.regions[0];
+	ASSERT_EQ(rgn->base, memblock_start_of_DRAM());
+	ASSERT_EQ(rgn->size, memblock_phys_mem_size() / 2);
+	ASSERT_EQ(memblock_get_region_node(rgn), 0);
+	rgn = &memblock.memory.regions[1];
+	ASSERT_EQ(rgn->base, memblock_start_of_DRAM() + memblock_phys_mem_size() / 2);
+	ASSERT_EQ(rgn->size, memblock_phys_mem_size() / 2);
+	ASSERT_EQ(memblock_get_region_node(rgn), 1);
+
+	/* Reserve 126 regions with the last one across node boundary */
+	for (i = 0; i < 125; i++)
+		memblock_reserve(memblock_start_of_DRAM() + SZ_16 * i, SZ_8);
+
+	memblock_reserve(memblock_start_of_DRAM() + memblock_phys_mem_size() / 2 - SZ_8,
+			SZ_16);
+
+	/*
+	 * Commit 61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")
+	 * do following process to set nid to each memblock.reserved region.
+	 * But it may miss some region if memblock_set_node() double the
+	 * array.
+	 *
+	 * By checking 'max', we make sure all region nid is set properly.
+	 */
+repeat:
+	max_reserved = memblock.reserved.max;
+	for_each_mem_region(rgn) {
+		int nid = memblock_get_region_node(rgn);
+
+		memblock_set_node(rgn->base, rgn->size, &memblock.reserved, nid);
+	}
+	if (max_reserved != memblock.reserved.max)
+		goto repeat;
+
+	/* Confirm each region has valid node set */
+	for_each_reserved_mem_region(rgn) {
+		ASSERT_TRUE(numa_valid_node(memblock_get_region_node(rgn)));
+		if (rgn == (memblock.reserved.regions + memblock.reserved.cnt - 1))
+			ASSERT_EQ(1, memblock_get_region_node(rgn));
+		else
+			ASSERT_EQ(0, memblock_get_region_node(rgn));
+	}
+
+	dummy_physical_memory_cleanup();
+
+	/*
+	 * The current reserved.regions is occupying a range of memory that
+	 * allocated from dummy_physical_memory_init(). After free the memory,
+	 * we must not use it. So restore the origin memory region to make sure
+	 * the tests can run as normal and not affected by the double array.
+	 */
+	memblock.reserved.regions = orig_region;
+	memblock.reserved.cnt = INIT_MEMBLOCK_RESERVED_REGIONS;
+
+	test_pass_pop();
+
+	return 0;
+}
+
+static int memblock_set_node_checks(void)
+{
+	prefix_reset();
+	prefix_push("memblock_set_node");
+	test_print("Running memblock_set_node tests...\n");
+
+	memblock_set_node_check();
+
+	prefix_pop();
+
+	return 0;
+}
+#else
+static int memblock_set_node_checks(void)
+{
+	return 0;
+}
+#endif
+
 int memblock_basic_checks(void)
 {
 	memblock_initialization_check();
@@ -2444,6 +2545,7 @@ int memblock_basic_checks(void)
 	memblock_bottom_up_checks();
 	memblock_trim_memory_checks();
 	memblock_overlaps_region_checks();
+	memblock_set_node_checks();
 
 	return 0;
 }
-- 
2.34.1

Re: [Patch v2 0/3] memblock: some fix for memmap_init_reserved_pages()

[Wei Yang]

Ping?

On Tue, Mar 18, 2025 at 07:19:45AM +0000, Wei Yang wrote:
>During code review, I found we may leave some reserved region with invalid nid
>when memblock_set_node() double the array. The patch set propose a fix and
>test case.
>
>Also I found memblock_set_node() accept size instead of end as second
>parameter. Currently it is misused.
>
>Patch 1: fix the second argument passed to memblock_set_node()
>Patch 2: fix the invalid node id we may have in memmap_init_reserved_pages()
>Patch 3: add a test case to verify it
>
>v2: move check out side of the loop
>
>Wei Yang (3):
>  mm/memblock: pass size instead of end to memblock_set_node()
>  mm/memblock: repeat setting reserved region nid if array is doubled
>  memblock tests: add test for memblock_set_node
>
> mm/memblock.c                            |  12 ++-
> tools/testing/memblock/tests/basic_api.c | 102 +++++++++++++++++++++++
> 2 files changed, 113 insertions(+), 1 deletion(-)
>
>-- 
>2.34.1

-- 
Wei Yang
Help you, Help me

Re: [Patch v2 0/3] memblock: some fix for memmap_init_reserved_pages()

[Mike Rapoport]

On Tue, 18 Mar 2025 07:19:45 +0000, Wei Yang wrote:
> During code review, I found we may leave some reserved region with invalid nid
> when memblock_set_node() double the array. The patch set propose a fix and
> test case.
> 
> Also I found memblock_set_node() accept size instead of end as second
> parameter. Currently it is misused.
> 
> [...]

Applied to for-next branch of memblock.git tree, thanks!

[1/3] mm/memblock: pass size instead of end to memblock_set_node()
      commit: 06eaa824fd239edd1eab2754f29b2d03da313003
[2/3] mm/memblock: repeat setting reserved region nid if array is doubled
      commit: eac8ea8736ccc09513152d970eb2a42ed78e87e8
[3/3] memblock tests: add test for memblock_set_node
      commit: 3b394dff15e14550a26b133fc7b556b5b526f6a5

tree: https://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock
branch: for-next

--
Sincerely yours,
Mike.