Re: [PATCH 01/49] mm/sparse: fix vmemmap accounting imbalance on memory hotplug error

[Mike Rapoport <rppt@kernel.org>]

Hi Muchun,

On Mon, Apr 13, 2026 at 08:47:45PM +0800, Muchun Song wrote:
> > On Apr 13, 2026, at 20:05, Mike Rapoport <rppt@kernel.org> wrote:
> >>>>> 
> >>>>> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> >>>>> index 6eadb9d116e4..ee27d0c0efe2 100644
> >>>>> --- a/mm/sparse-vmemmap.c
> >>>>> +++ b/mm/sparse-vmemmap.c
> >>>>> @@ -822,11 +822,11 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn,
> >>>>> return pfn_to_page(pfn);
> >>>>> 
> >>>>> memmap = populate_section_memmap(pfn, nr_pages, nid, altmap, pgmap);
> >>>>> + memmap_pages_add(DIV_ROUND_UP(nr_pages * sizeof(struct page), PAGE_SIZE));
> >>>> 
> >>>> This logically belongs to success path in populate_section_memmap(). If we
> >>>> update the counter there, we won't need to temporarily increase it at all.
> >>> 
> >>> Not strictly related to this patchset, but it seems, we can have a single
> >>> memmap_boot_pages_add() in memmap_alloc() rather than to update the counter
> >>> in memmap_alloc() callers.
> >> 
> >> It will indeed become simpler and is a good cleanup direction, but there
> >> is a slight change in semantics: the page tables used for vmemmap page
> >> mapping will also be counted in memmap_boot_pages_add(). This might not
> >> be an issue (after all, the size of the page tables is very small compared
> >> to struct pages, right?).
> >> 
> >> Additionally, I still lean toward making no changes to this patch, because
> >> this is a pure bugfix patch — of course, it is meant to facilitate backporting
> >> for those who need it. The cleanup would involve many more changes, so I
> >> prefer to do that in a separate patch. What do you think?
> > 
> > For this patch and easy backporting I still think that cleaner to have the
> > counter incremented in populate_section_memmap() rather immediately after
> > it.
> 
> Hi Mike,
> 
> Alright, let’s revisit your solution. After we’ve moved the counter into the 
> populate_section_memmap(), we still need to increase the counter temporarily
> (but in populate_section_memmap()) even if we fail to populate. That’s
> because section_deactivate() reduces the counter without exception, isn’t it?
> Just want to make sure we are on the same page on the meaning of “temporarily
> increase”.  Maybe you do not mean “temporarily” in this case.

I suggest to increase the counter only if we succeeded to populate:

diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 6eadb9d116e4..247fd54f1003 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -656,7 +656,13 @@ static struct page * __meminit populate_section_memmap(unsigned long pfn,
 		unsigned long nr_pages, int nid, struct vmem_altmap *altmap,
 		struct dev_pagemap *pgmap)
 {
-	return __populate_section_memmap(pfn, nr_pages, nid, altmap, pgmap);
+	struct page *p = __populate_section_memmap(pfn, nr_pages, nid, altmap,
+						   pgmap);
+
+	if (p)
+		memmap_pages_add(DIV_ROUND_UP(nr_pages * sizeof(struct page), PAGE_SIZE));
+
+	return p;
 }
 
 static void depopulate_section_memmap(unsigned long pfn, unsigned long nr_pages,
@@ -826,7 +832,6 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn,
 		section_deactivate(pfn, nr_pages, altmap);
 		return ERR_PTR(-ENOMEM);
 	}
-	memmap_pages_add(DIV_ROUND_UP(nr_pages * sizeof(struct page), PAGE_SIZE));
 
 	return memmap;
 }
 

Then we'll better follow "all or nothing" principle and won't have
exceptional cases in section_deactivate().

> Thanks,
> Muchun.

-- 
Sincerely yours,
Mike.