Re: [PATCH v4 02/14] memblock: add MEMBLOCK_RSRV_KERN flag

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

On Wed, Feb 19, 2025 at 09:24:31AM +0200, Mike Rapoport wrote:
>Hi,
>
>On Tue, Feb 18, 2025 at 03:50:04PM +0000, Wei Yang wrote:
>> On Thu, Feb 06, 2025 at 03:27:42PM +0200, Mike Rapoport wrote:
>> >From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>> >
>> >to denote areas that were reserved for kernel use either directly with
>> >memblock_reserve_kern() or via memblock allocations.
>> >
>> >Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
>> >---
>> > include/linux/memblock.h | 16 +++++++++++++++-
>> > mm/memblock.c            | 32 ++++++++++++++++++++++++--------
>> > 2 files changed, 39 insertions(+), 9 deletions(-)
>> >
>> >diff --git a/include/linux/memblock.h b/include/linux/memblock.h
>> >index e79eb6ac516f..65e274550f5d 100644
>> >--- a/include/linux/memblock.h
>> >+++ b/include/linux/memblock.h
>> >@@ -50,6 +50,7 @@ enum memblock_flags {
>> > 	MEMBLOCK_NOMAP		= 0x4,	/* don't add to kernel direct mapping */
>> > 	MEMBLOCK_DRIVER_MANAGED = 0x8,	/* always detected via a driver */
>> > 	MEMBLOCK_RSRV_NOINIT	= 0x10,	/* don't initialize struct pages */
>> >+	MEMBLOCK_RSRV_KERN	= 0x20,	/* memory reserved for kernel use */
>> 
>> Above memblock_flags, there are comments on explaining those flags.
>> 
>> Seems we miss it for MEMBLOCK_RSRV_KERN.
>
>Right, thanks!
> 
>> > 
>> > #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
>> >@@ -1459,14 +1460,14 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
>> > again:
>> > 	found = memblock_find_in_range_node(size, align, start, end, nid,
>> > 					    flags);
>> >-	if (found && !memblock_reserve(found, size))
>> >+	if (found && !__memblock_reserve(found, size, nid, MEMBLOCK_RSRV_KERN))
>> 
>> Maybe we could use memblock_reserve_kern() directly. If my understanding is
>> correct, the reserved region's nid is not used.
>
>We use nid of reserved regions in reserve_bootmem_region() (commit
>61167ad5fecd ("mm: pass nid to reserve_bootmem_region()")) but KHO needs to
>know the distribution of reserved memory among the nodes before
>memmap_init_reserved_pages().
> 

I took another look into this commit. There maybe a very corner case in which
will leave a reserved region with no nid set.

memmap_init_reserved_pages()
    for_each_mem_region() {
        ...
	memblock_set_node(start, end, &memblock.reserved, nid);
    }

We leverage the iteration here to set nid to all regions in memblock.reserved.
But memblock_set_node() may call memblock_double_array() to expand the array,
which may get a range before current start. So we would miss to set the
correct nid to the new reserved region.

I have tried to create a case in memblock test. This would happen when there
are 126 memblock.reserved regions. And the last region is across the last two
node.

One way to fix this is compare type->max in memblock_set_node(). Then check
this return value in memmap_init_reserved_pages(). If we found the size
changes, repeat the iteration.

But this is a very trivial one, not sure it worth fix.

-- 
Wei Yang
Help you, Help me