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

Mike Rapoport
2026-04-13T13:36:02Z

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.