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@xxxxxxxxx>
> >> CC: Mike Rapoport <rppt@xxxxxxxxxx>
> >> CC: Yajun Deng <yajun.deng@xxxxxxxxx>
> >> CC: <stable@xxxxxxxxxxxxxxx>
> >> ---
> >> 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.
