Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 17 Jun 2023 11:12:49 +0300
From:   Mike Rapoport <rppt@kernel.org>
To:     Yajun Deng <yajun.deng@linux.dev>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v2] mm: pass nid to reserve_bootmem_region()
Message-ID: <20230617081249.GW52412@kernel.org>
References: <20230616072247.GL52412@kernel.org>
 <20230616023011.2952211-1-yajun.deng@linux.dev>
 <5ba9ad9bedb2fd3fb96571a778fc35b5@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5ba9ad9bedb2fd3fb96571a778fc35b5@linux.dev>
Precedence: bulk

On Fri, Jun 16, 2023 at 07:51:21AM +0000, Yajun Deng wrote:
> June 16, 2023 3:22 PM, "Mike Rapoport" <rppt@kernel.org> wrote:
> > On Fri, Jun 16, 2023 at 10:30:11AM +0800, Yajun Deng wrote:
> > 
> >> diff --git a/mm/mm_init.c b/mm/mm_init.c
> >> index d393631599a7..1499efbebc6f 100644
> >> --- a/mm/mm_init.c
> >> +++ b/mm/mm_init.c
> >> @@ -738,16 +735,20 @@ static inline void init_reserved_page(unsigned long pfn)
> >> * marks the pages PageReserved. The remaining valid pages are later
> >> * sent to the buddy page allocator.
> >> */
> >> -void __meminit reserve_bootmem_region(phys_addr_t start, phys_addr_t end)
> >> +void __meminit reserve_bootmem_region(phys_addr_t start,
> >> + phys_addr_t end, int nid)
> >> {
> >> unsigned long start_pfn = PFN_DOWN(start);
> >> unsigned long end_pfn = PFN_UP(end);
> >> 
> >> + if (nid == MAX_NUMNODES)
> >> + nid = first_online_node;
> > 
> > How can this happen?
> > 
> 
> Some reserved memory regions may not set nid. I found it when I debug.
> We can see that by memblock_debug_show().
 
Hmm, indeed. But then it means that some struct pages for the reserved pages
will get wrong nid and if they are freed we'd actually get pages with wrong
nid. 

Maybe it's this time to set nid on all reserved pages with something like

diff --git a/mm/memblock.c b/mm/memblock.c
index 3feafea06ab2..fcd0987e2496 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2084,6 +2084,14 @@ static void __init memmap_init_reserved_pages(void)
 	phys_addr_t start, end;
 	u64 i;
 
+	for_each_mem_region(region) {
+		int nid = memblock_get_region_node(region);
+
+		start = region->base;
+		end = start + region->size;
+		memblock_set_node(start, end, &memblock.reserved, nid);
+	}
+
 	/* initialize struct pages for the reserved regions */
 	for_each_reserved_mem_range(i, &start, &end)
 		reserve_bootmem_region(start, end);

> >> @@ -2579,7 +2580,13 @@ void __init set_dma_reserve(unsigned long new_dma_reserve)
> >> void __init memblock_free_pages(struct page *page, unsigned long pfn,
> >> unsigned int order)
> >> {
> >> - if (!early_page_initialised(pfn))
> >> + int nid = 0;
> >> +
> >> +#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> >> + nid = early_pfn_to_nid(pfn);
> >> +#endif

Please replace #ifdef with 

	if (IS_DEFINED(CONFIG_DEFERRED_STRUCT_PAGE_INIT))
 
> > Wen can pass nid to memblock_free_pages, no?
> >
> 
> memblock_free_pages() was called by __free_pages_memory() and memblock_free_late().
> For the latter, I'm not sure if we can pass nid.
> 
> I think we can pass nid to reserve_bootmem_region() in this patch, and pass nid to
> memblock_free_pages() in another patch if we can confirm this.

Fair enough.

-- 
Sincerely yours,
Mike.