diff mbox series

[v4,01/14] mm/mm_init: rename init_reserved_page to init_deferred_page

Message ID 20250206132754.2596694-2-rppt@kernel.org (mailing list archive)
State New
Headers show
Series kexec: introduce Kexec HandOver (KHO) | expand

Commit Message

Mike Rapoport Feb. 6, 2025, 1:27 p.m. UTC 
From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>

When CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, init_reserved_page()
function performs initialization of a struct page that would have been
deferred normally.

Rename it to init_deferred_page() to better reflect what the function does.

Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
 mm/mm_init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Wei Yang Feb. 18, 2025, 2:59 p.m. UTC | #1 
On Thu, Feb 06, 2025 at 03:27:41PM +0200, Mike Rapoport wrote:
>From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
>When CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, init_reserved_page()
>function performs initialization of a struct page that would have been
>deferred normally.
>
>Rename it to init_deferred_page() to better reflect what the function does.

Would it be confused with deferred_init_pages()?

And it still calls __init_reserved_page_zone(), even we __SetPageReserved()
after it. Current logic looks not clear.
Mike Rapoport Feb. 19, 2025, 7:13 a.m. UTC | #2 
Hi,

On Tue, Feb 18, 2025 at 02:59:04PM +0000, Wei Yang wrote:
> On Thu, Feb 06, 2025 at 03:27:41PM +0200, Mike Rapoport wrote:
> >From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> >
> >When CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, init_reserved_page()
> >function performs initialization of a struct page that would have been
> >deferred normally.
> >
> >Rename it to init_deferred_page() to better reflect what the function does.
> 
> Would it be confused with deferred_init_pages()?

Why? It initializes a single page, deferred_init_pages() initializes many.

> And it still calls __init_reserved_page_zone(), even we __SetPageReserved()
> after it. Current logic looks not clear.

There's no __init_reserved_page_zone(). Currently init_reserved_page()
detects the zone of the page and calls __init_single_page(), so essentially
it initializes one struct page.

And we __SetPageReserved() in reserve_bootmem_region() after call to
init_reseved_page() because pages there are indeed reserved.
 
> -- 
> Wei Yang
> Help you, Help me
Wei Yang Feb. 20, 2025, 8:36 a.m. UTC | #3 
On Wed, Feb 19, 2025 at 09:13:22AM +0200, Mike Rapoport wrote:
>Hi,
>
>On Tue, Feb 18, 2025 at 02:59:04PM +0000, Wei Yang wrote:
>> On Thu, Feb 06, 2025 at 03:27:41PM +0200, Mike Rapoport wrote:
>> >From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>> >
>> >When CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, init_reserved_page()
>> >function performs initialization of a struct page that would have been
>> >deferred normally.
>> >
>> >Rename it to init_deferred_page() to better reflect what the function does.
>> 
>> Would it be confused with deferred_init_pages()?
>
>Why? It initializes a single page, deferred_init_pages() initializes many.
>

See below.

>> And it still calls __init_reserved_page_zone(), even we __SetPageReserved()
>> after it. Current logic looks not clear.
>
>There's no __init_reserved_page_zone(). Currently init_reserved_page()
>detects the zone of the page and calls __init_single_page(), so essentially
>it initializes one struct page.
>
>And we __SetPageReserved() in reserve_bootmem_region() after call to
>init_reseved_page() because pages there are indeed reserved.
> 

Hmm... I am not sure we are looking at the same code. I take a look at current
mm-unstable, this patch set is not included.  So I am looking at previous
version with this last commit:

  8bf30f9d23eb 2025-02-06 Documentation: KHO: add memblock bindings

Here is what I see for init_deferred_page()'s definition:

init_deferred_page()
  __init_deferred_page()
    __init_reserved_page_zone()   <--- I do see this function, it is removed?
      __init_single_page()

What I want to say is __init_deferred_page() calls
__init_reserved_page_zone(). This sounds imply a deferred page is always
reserved page. But we know it is not.  deferred_init_pages() initialize the
pages are not reserved one. Or we want to have this context in
__init_deferred_page()?

>-- 
>Sincerely yours,
>Mike.
Mike Rapoport Feb. 20, 2025, 2:54 p.m. UTC | #4 
On Thu, Feb 20, 2025 at 08:36:01AM +0000, Wei Yang wrote:
> On Wed, Feb 19, 2025 at 09:13:22AM +0200, Mike Rapoport wrote:
> >Hi,
> >
> >On Tue, Feb 18, 2025 at 02:59:04PM +0000, Wei Yang wrote:
> >> On Thu, Feb 06, 2025 at 03:27:41PM +0200, Mike Rapoport wrote:
> >> >From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> >> >
> >> >When CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, init_reserved_page()
> >> >function performs initialization of a struct page that would have been
> >> >deferred normally.
> >> >
> >> >Rename it to init_deferred_page() to better reflect what the function does.
> >> 
> >> Would it be confused with deferred_init_pages()?
> >
> >Why? It initializes a single page, deferred_init_pages() initializes many.
> >
> 
> See below.
> 
> >> And it still calls __init_reserved_page_zone(), even we __SetPageReserved()
> >> after it. Current logic looks not clear.
> >
> >There's no __init_reserved_page_zone(). Currently init_reserved_page()
> >detects the zone of the page and calls __init_single_page(), so essentially
> >it initializes one struct page.
> >
> >And we __SetPageReserved() in reserve_bootmem_region() after call to
> >init_reseved_page() because pages there are indeed reserved.
> > 
> 
> Hmm... I am not sure we are looking at the same code. I take a look at current
> mm-unstable, this patch set is not included.

I was looking at Linus tree, it was not there yet :)

> So I am looking at previous version with this last commit:
> 
>   8bf30f9d23eb 2025-02-06 Documentation: KHO: add memblock bindings
> 
> Here is what I see for init_deferred_page()'s definition:
> 
> init_deferred_page()
>   __init_deferred_page()
>     __init_reserved_page_zone()   <--- I do see this function, it is removed?
>       __init_single_page()
> 
> What I want to say is __init_deferred_page() calls
> __init_reserved_page_zone(). This sounds imply a deferred page is always
> reserved page. But we know it is not.  deferred_init_pages() initialize the
> pages are not reserved one. Or we want to have this context in
> __init_deferred_page()?

If the commit that introduced __init_reserved_page_zone goes in before KHO,
I'll just rename both functions, there is nothing about reserved pages
there.
 
> >-- 
> >Sincerely yours,
> >Mike.
> 
> -- 
> Wei Yang
> Help you, Help me
diff mbox series

Patch

diff --git a/mm/mm_init.c b/mm/mm_init.c
index 2630cc30147e..c4b425125bad 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -705,7 +705,7 @@  defer_init(int nid, unsigned long pfn, unsigned long end_pfn)
 	return false;
 }
 
-static void __meminit init_reserved_page(unsigned long pfn, int nid)
+static void __meminit init_deferred_page(unsigned long pfn, int nid)
 {
 	pg_data_t *pgdat;
 	int zid;
@@ -739,7 +739,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, int nid)
+static inline void init_deferred_page(unsigned long pfn, int nid)
 {
 }
 #endif /* CONFIG_DEFERRED_STRUCT_PAGE_INIT */
@@ -760,7 +760,7 @@  void __meminit reserve_bootmem_region(phys_addr_t start,
 		if (pfn_valid(start_pfn)) {
 			struct page *page = pfn_to_page(start_pfn);
 
-			init_reserved_page(start_pfn, nid);
+			init_deferred_page(start_pfn, nid);
 
 			/*
 			 * no need for atomic set_bit because the struct