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

[Wei Yang <richard.weiyang@gmail.com>]

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