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

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

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                            |  14 +++-
 tools/testing/memblock/tests/basic_api.c | 102 +++++++++++++++++++++++
 2 files changed, 115 insertions(+), 1 deletion(-)

-- 
2.34.1

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

[PATCH 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>
---
 mm/memblock.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/memblock.c b/mm/memblock.c
index 85442f1b7f14..302dd7bc622d 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2184,7 +2184,10 @@ static void __init memmap_init_reserved_pages(void)
 	 * set nid on all reserved pages and also treat struct
 	 * pages for the NOMAP regions as PageReserved
 	 */
+repeat:
 	for_each_mem_region(region) {
+		unsigned long max = memblock.reserved.max;
+
 		nid = memblock_get_region_node(region);
 		start = region->base;
 		end = start + region->size;
@@ -2193,6 +2196,15 @@ static void __init memmap_init_reserved_pages(void)
 			reserve_bootmem_region(start, end, nid);
 
 		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 != memblock.reserved.max)
+			goto repeat;
 	}
 
 	/*
-- 
2.34.1

[PATCH 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>
---
 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..45dd122a62a8 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;
+	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:
+	for_each_mem_region(rgn) {
+		int nid = memblock_get_region_node(rgn);
+		unsigned long max = memblock.reserved.max;
+
+		memblock_set_node(rgn->base, rgn->size, &memblock.reserved, nid);
+		if (max != 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 2/3] mm/memblock: repeat setting reserved region nid if array is doubled

[Mike Rapoport]

Hi Wei,

On Wed, Mar 12, 2025 at 01:07:27PM +0000, 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.
> 
> 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>
> ---
>  mm/memblock.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 85442f1b7f14..302dd7bc622d 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2184,7 +2184,10 @@ static void __init memmap_init_reserved_pages(void)
>  	 * set nid on all reserved pages and also treat struct
>  	 * pages for the NOMAP regions as PageReserved
>  	 */
> +repeat:
>  	for_each_mem_region(region) {
> +		unsigned long max = memblock.reserved.max;
> +
>  		nid = memblock_get_region_node(region);
>  		start = region->base;
>  		end = start + region->size;
> @@ -2193,6 +2196,15 @@ static void __init memmap_init_reserved_pages(void)
>  			reserve_bootmem_region(start, end, nid);
>  
>  		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 != memblock.reserved.max)
> +			goto repeat;

This check can be moved outside the loop, can't it?

>  	}
>  
>  	/*
> -- 
> 2.34.1
> 

-- 
Sincerely yours,
Mike.

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

[Wei Yang]

On Thu, Mar 13, 2025 at 05:07:59PM +0200, Mike Rapoport wrote:
>Hi Wei,
>
>On Wed, Mar 12, 2025 at 01:07:27PM +0000, 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.
>> 
>> 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>
>> ---
>>  mm/memblock.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index 85442f1b7f14..302dd7bc622d 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -2184,7 +2184,10 @@ static void __init memmap_init_reserved_pages(void)
>>  	 * set nid on all reserved pages and also treat struct
>>  	 * pages for the NOMAP regions as PageReserved
>>  	 */
>> +repeat:
>>  	for_each_mem_region(region) {
>> +		unsigned long max = memblock.reserved.max;
>> +
>>  		nid = memblock_get_region_node(region);
>>  		start = region->base;
>>  		end = start + region->size;
>> @@ -2193,6 +2196,15 @@ static void __init memmap_init_reserved_pages(void)
>>  			reserve_bootmem_region(start, end, nid);
>>  
>>  		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 != memblock.reserved.max)
>> +			goto repeat;
>
>This check can be moved outside the loop, can't it?
>

We can. You mean something like this?

diff --git a/mm/memblock.c b/mm/memblock.c
index 85442f1b7f14..67fd1695cce4 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;
 
 	/*
 	 * set nid on all reserved pages and also treat struct
 	 * pages for the NOMAP regions as PageReserved
 	 */
+repeat:
+	max = 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 != memblock.reserved.max)
+		goto repeat;
 
 	/*
 	 * initialize struct pages for reserved regions that don't have
-- 
Wei Yang
Help you, Help me

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

[Mike Rapoport]

On Fri, Mar 14, 2025 at 02:03:51AM +0000, Wei Yang wrote:
> On Thu, Mar 13, 2025 at 05:07:59PM +0200, Mike Rapoport 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.
> >> 
> >> 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>
> >> ---
> >>  mm/memblock.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >> 
> >> diff --git a/mm/memblock.c b/mm/memblock.c
> >> index 85442f1b7f14..302dd7bc622d 100644
> >> --- a/mm/memblock.c
> >> +++ b/mm/memblock.c
> >> @@ -2184,7 +2184,10 @@ static void __init memmap_init_reserved_pages(void)
> >>  	 * set nid on all reserved pages and also treat struct
> >>  	 * pages for the NOMAP regions as PageReserved
> >>  	 */
> >> +repeat:
> >>  	for_each_mem_region(region) {
> >> +		unsigned long max = memblock.reserved.max;
> >> +
> >>  		nid = memblock_get_region_node(region);
> >>  		start = region->base;
> >>  		end = start + region->size;
> >> @@ -2193,6 +2196,15 @@ static void __init memmap_init_reserved_pages(void)
> >>  			reserve_bootmem_region(start, end, nid);
> >>  
> >>  		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 != memblock.reserved.max)
> >> +			goto repeat;
> >
> >This check can be moved outside the loop, can't it?
> >
> 
> We can. You mean something like this?

Yes, something like this.
 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 85442f1b7f14..67fd1695cce4 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;

maybe max_reserved?
>  
>  	/*
>  	 * set nid on all reserved pages and also treat struct
>  	 * pages for the NOMAP regions as PageReserved
>  	 */
> +repeat:
> +	max = 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 != memblock.reserved.max)
> +		goto repeat;
>  
>  	/*
>  	 * initialize struct pages for reserved regions that don't have
> -- 
> Wei Yang
> Help you, Help me

-- 
Sincerely yours,
Mike.

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

[Wei Yang]

On Sun, Mar 16, 2025 at 05:32:35PM +0200, Mike Rapoport wrote:
>On Fri, Mar 14, 2025 at 02:03:51AM +0000, Wei Yang wrote:
>> On Thu, Mar 13, 2025 at 05:07:59PM +0200, Mike Rapoport 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.
>> >> 
>> >> 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>
>> >> ---
>> >>  mm/memblock.c | 12 ++++++++++++
>> >>  1 file changed, 12 insertions(+)
>> >> 
>> >> diff --git a/mm/memblock.c b/mm/memblock.c
>> >> index 85442f1b7f14..302dd7bc622d 100644
>> >> --- a/mm/memblock.c
>> >> +++ b/mm/memblock.c
>> >> @@ -2184,7 +2184,10 @@ static void __init memmap_init_reserved_pages(void)
>> >>  	 * set nid on all reserved pages and also treat struct
>> >>  	 * pages for the NOMAP regions as PageReserved
>> >>  	 */
>> >> +repeat:
>> >>  	for_each_mem_region(region) {
>> >> +		unsigned long max = memblock.reserved.max;
>> >> +
>> >>  		nid = memblock_get_region_node(region);
>> >>  		start = region->base;
>> >>  		end = start + region->size;
>> >> @@ -2193,6 +2196,15 @@ static void __init memmap_init_reserved_pages(void)
>> >>  			reserve_bootmem_region(start, end, nid);
>> >>  
>> >>  		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 != memblock.reserved.max)
>> >> +			goto repeat;
>> >
>> >This check can be moved outside the loop, can't it?
>> >
>> 
>> We can. You mean something like this?
>
>Yes, something like this.
> 
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index 85442f1b7f14..67fd1695cce4 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;
>
>maybe max_reserved?

Sure, will update it.

>>  
>>  	/*
>>  	 * set nid on all reserved pages and also treat struct
>>  	 * pages for the NOMAP regions as PageReserved
>>  	 */
>> +repeat:
>> +	max = 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 != memblock.reserved.max)
>> +		goto repeat;
>>  
>>  	/*
>>  	 * initialize struct pages for reserved regions that don't have
>> -- 
>> Wei Yang
>> Help you, Help me
>
>-- 
>Sincerely yours,
>Mike.

-- 
Wei Yang
Help you, Help me