Re: [PATCH v4 2/2] mm: Init page count in reserve_bootmem_region when MEMINIT_EARLY

[Mike Rapoport]

On Mon, Oct 02, 2023 at 03:03:56PM +0800, Yajun Deng wrote:
> 
> On 2023/10/2 02:59, Mike Rapoport wrote:
> > On Fri, Sep 29, 2023 at 06:27:25PM +0800, Yajun Deng wrote:
> > > On 2023/9/29 18:02, Mike Rapoport wrote:
> > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > > > > index 06be8821d833..b868caabe8dc 100644
> > > > > > > --- a/mm/page_alloc.c
> > > > > > > +++ b/mm/page_alloc.c
> > > > > > > @@ -1285,18 +1285,22 @@ void __free_pages_core(struct page *page, unsigned int order)
> > > > > > >     	unsigned int loop;
> > > > > > >     	/*
> > > > > > > -	 * When initializing the memmap, __init_single_page() sets the refcount
> > > > > > > -	 * of all pages to 1 ("allocated"/"not free"). We have to set the
> > > > > > > -	 * refcount of all involved pages to 0.
> > > > > > > +	 * When initializing the memmap, memmap_init_range sets the refcount
> > > > > > > +	 * of all pages to 1 ("reserved" and "free") in hotplug context. We
> > > > > > > +	 * have to set the refcount of all involved pages to 0. Otherwise,
> > > > > > > +	 * we don't do it, as reserve_bootmem_region only set the refcount on
> > > > > > > +	 * reserve region ("reserved") in early context.
> > > > > > >     	 */
> > > > > > Again, why hotplug and early init should be different?
> > > > > I will add a comment that describes it will save boot time.
> > > > But why do we need initialize struct pages differently at boot time vs
> > > > memory hotplug?
> > > > Is there a reason memory hotplug cannot have page count set to 0 just like
> > > > for pages reserved at boot time?
> > > This patch just save boot time in MEMINIT_EARLY. If someone finds out that
> > > it can save time in
> > > 
> > > MEMINIT_HOTPLUG, I think it can be done in another patch later. I just
> > > keeping it in the same.
> > But it's not the same. It becomes slower after your patch and the code that
> > frees the pages for MEMINIT_EARLY and MEMINIT_HOTPLUG becomes non-uniform
> > for no apparent reason.
> 
> __free_pages_core will also be called by others, such as:
> deferred_free_range, do_collection and memblock_free_late.
> 
> We couldn't remove  'if (page_count(page))' even if we set page count to 0
> when MEMINIT_HOTPLUG.

That 'if' breaks the invariant that __free_pages_core is always called for
pages with initialized page count. Adding it may lead to subtle bugs and
random memory corruption so we don't want to add it at the first place.

-- 
Sincerely yours,
Mike.