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...
