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

From: Yajun Deng
Date: Tue Oct 03 2023 - 10:38:24 EST



On 2023/10/2 19:25, David Hildenbrand wrote:
On 02.10.23 13:10, Mike Rapoport wrote:
On Mon, Oct 02, 2023 at 10:56:51AM +0200, David Hildenbrand wrote:
On 02.10.23 10:47, Mike Rapoport wrote:
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.

As long as we have to special-case memory hotplug, we know that we are
always coming via generic_online_page() in that case. We could either move
some logic over there, or let __free_pages_core() know what it should do.

Looks like the patch rather special cases MEMINIT_EARLY, although I didn't
check throughfully other code paths.
Anyway, relying on page_count() to be correct in different ways for
different callers of __free_pages_core() does not sound right to me.

Absolutely agreed.

I already sent v5  a few days ago. Comments, please...