Message-ID: <5ba9ad9bedb2fd3fb96571a778fc35b5@linux.dev>
Date:   Fri, 16 Jun 2023 07:51:21 +0000
From:   "Yajun Deng" <yajun.deng@...ux.dev>
To:     "Mike Rapoport" <rppt@...nel.org>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, "kernel test robot" <lkp@...el.com>
Subject: Re: [PATCH v2] mm: pass nid to reserve_bootmem_region()

June 16, 2023 3:22 PM, "Mike Rapoport" <rppt@...nel.org> wrote:

> On Fri, Jun 16, 2023 at 10:30:11AM +0800, Yajun Deng wrote:
> 
>> early_pfn_to_nid() is called frequently in init_reserved_page(), it
>> returns the node id of the PFN. These PFN are probably from the same
>> memory region, they have the same node id. It's not necessary to call
>> early_pfn_to_nid() for each PFN.
>> 
>> Pass nid to eserve_bootmem_region() and drop the call to
>> early_pfn_to_nid() in init_reserved_page().
>> 
>> The most beneficial function is memmap_init_reserved_pages() if define
>> CONFIG_DEFERRED_STRUCT_PAGE_INIT.
>> The following data was tested on x86 machine, it has 190GB RAM,
>> 
>> before:
>> memmap_init_reserved_pages() 67ms
>> 
>> after:
>> memmap_init_reserved_pages() 20ms
>> 
>> Signed-off-by: Yajun Deng <yajun.deng@...ux.dev>
>> Reported-by: kernel test robot <lkp@...el.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202306160145.juJMr3Bi-lkp@intel.com
>> ---
>> include/linux/mm.h | 3 ++-
>> mm/memblock.c | 9 ++++++---
>> mm/mm_init.c | 31 +++++++++++++++++++------------
>> 3 files changed, 27 insertions(+), 16 deletions(-)
>> 
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 17317b1673b0..39e72ca6bf22 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -2964,7 +2964,8 @@ extern unsigned long free_reserved_area(void *start, void *end,
>> 
>> extern void adjust_managed_page_count(struct page *page, long count);
>> 
>> -extern void reserve_bootmem_region(phys_addr_t start, phys_addr_t end);
>> +extern void reserve_bootmem_region(phys_addr_t start,
>> + phys_addr_t end, int nid);
>> 
>> /* Free the reserved page into the buddy system, so it gets managed. */
>> static inline void free_reserved_page(struct page *page)
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index ff0da1858778..6dc51dc845e5 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -2091,18 +2091,21 @@ static void __init memmap_init_reserved_pages(void)
>> {
>> struct memblock_region *region;
>> phys_addr_t start, end;
>> + int nid;
>> u64 i;
>> 
>> /* initialize struct pages for the reserved regions */
>> - for_each_reserved_mem_range(i, &start, &end)
>> - reserve_bootmem_region(start, end);
>> + __for_each_mem_range(i, &memblock.reserved, NULL, NUMA_NO_NODE,
>> + MEMBLOCK_NONE, &start, &end, &nid)
>> + reserve_bootmem_region(start, end, nid);
> 
> I'd prefer to see for_each_reserved_mem_region() loop here
> 
okay.

>> /* and also treat struct pages for the NOMAP regions as PageReserved */
>> for_each_mem_region(region) {
>> if (memblock_is_nomap(region)) {
>> start = region->base;
>> end = start + region->size;
>> - reserve_bootmem_region(start, end);
>> + nid = memblock_get_region_node(region);
>> + reserve_bootmem_region(start, end, nid);
>> }
>> }
>> }
>> 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
>> @@ -646,10 +646,8 @@ static inline void pgdat_set_deferred_range(pg_data_t *pgdat)
>> }
>> 
>> /* Returns true if the struct page for the pfn is initialised */
>> -static inline bool __meminit early_page_initialised(unsigned long pfn)
>> +static inline bool __meminit early_page_initialised(unsigned long pfn, int nid)
>> {
>> - int nid = early_pfn_to_nid(pfn);
>> -
>> if (node_online(nid) && pfn >= NODE_DATA(nid)->first_deferred_pfn)
>> return false;
>> 
>> @@ -695,15 +693,14 @@ defer_init(int nid, unsigned long pfn, unsigned long end_pfn)
>> return false;
>> }
>> 
>> -static void __meminit init_reserved_page(unsigned long pfn)
>> +static void __meminit init_reserved_page(unsigned long pfn, int nid)
>> {
>> pg_data_t *pgdat;
>> - int nid, zid;
>> + int zid;
>> 
>> - if (early_page_initialised(pfn))
>> + if (early_page_initialised(pfn, nid))
>> return;
>> 
>> - nid = early_pfn_to_nid(pfn);
>> pgdat = NODE_DATA(nid);
>> 
>> for (zid = 0; zid < MAX_NR_ZONES; zid++) {
>> @@ -717,7 +714,7 @@ static void __meminit init_reserved_page(unsigned long pfn)
>> #else
>> static inline void pgdat_set_deferred_range(pg_data_t *pgdat) {}
>> 
>> -static inline bool early_page_initialised(unsigned long pfn)
>> +static inline bool early_page_initialised(unsigned long pfn, int nid)
>> {
>> return true;
>> }
>> @@ -727,7 +724,7 @@ static inline bool defer_init(int nid, unsigned long pfn, unsigned long
>> end_pfn)
>> return false;
>> }
>> 
>> -static inline void init_reserved_page(unsigned long pfn)
>> +static inline void init_reserved_page(unsigned long pfn, int nid)
>> {
>> }
>> #endif /* CONFIG_DEFERRED_STRUCT_PAGE_INIT */
>> @@ -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().

>> +
>> for (; start_pfn < end_pfn; start_pfn++) {
>> if (pfn_valid(start_pfn)) {
>> struct page *page = pfn_to_page(start_pfn);
>> 
>> - init_reserved_page(start_pfn);
>> + init_reserved_page(start_pfn, nid);
>> 
>> /* Avoid false-positive PageTail() */
>> INIT_LIST_HEAD(&page->lru);
>> @@ -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
> 
> 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.
 
>> +
>> + if (!early_page_initialised(pfn, nid))
>> return;
>> if (!kmsan_memblock_free_pages(page, order)) {
>> /* KMSAN will take care of these pages. */
>> --
>> 2.25.1
> 
> --
> Sincerely yours,
> Mike.

Powered by blists - more mailing lists