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

[Mike Rapoport <rppt@kernel.org>]

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.

By "currently" I meant the Linus tree.

> 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()?

In mm-unstable the code is slightly reorganized, but in the end it still
initializes a deferred page. It's just only called for reserved page, but
the initialization itself does not presume that the page is reserved.

So both functions are misnamed here,  __init_reserved_page_zone() and
init_reserved_page().

> -- 
> Wei Yang
> Help you, Help me

-- 
Sincerely yours,
Mike.