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

[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

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