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

[Muchun Song]

Sent from my iPad

> On Apr 13, 2026, at 20:05, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> 
> On Mon, Apr 13, 2026 at 05:49:17PM +0800, Muchun Song wrote:
>> 
>> 
>>>> On Apr 13, 2026, at 17:35, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
>>> 
>>> On Mon, Apr 13, 2026 at 12:19:50PM +0300, Mike Rapoport wrote:
>>>> On Sun, Apr 05, 2026 at 08:51:52PM +0800, Muchun Song wrote:
>>>>> In section_activate(), if populate_section_memmap() fails, the error
>>>>> handling path calls section_deactivate() to roll back the state. This
>>>>> approach introduces an accounting imbalance.
>>>>> 
>>>>> Since the commit c3576889d87b ("mm: fix accounting of memmap pages"),
>>>>> memmap pages are accounted for only after populate_section_memmap()
>>>>> succeeds. However, section_deactivate() unconditionally decrements the
>>>>> vmemmap account. Consequently, a failure in populate_section_memmap()
>>>>> leads to a negative offset (underflow) in the system's vmemmap tracking.
>>>>> 
>>>>> We can fix this by ensuring that the vmemmap accounting is incremented
>>>>> immediately before checking for the success of populate_section_memmap().
>>>>> If populate_section_memmap() fails, the subsequent call to
>>>>> section_deactivate() will decrement the accounting, perfectly offsetting
>>>>> the increment and maintaining balance.
>>>>> 
>>>>> Fixes: c3576889d87b ("mm: fix accounting of memmap pages")
>>>>> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>>>>> ---
>>>>> mm/sparse-vmemmap.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> 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.

Thanks,
Muchun.

> 
>> Thanks,
>> Muchun.
>> 
>>> 
>>>>> if (!memmap) {
>>>>> 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;
>>>>> }
>>>>> --
>>>>> 2.20.1
>>>>> 
>>>> 
>>>> --
>>>> Sincerely yours,
>>>> Mike.
>>> 
>>> --
>>> Sincerely yours,
>>> Mike.
>> 
>> 
>> 
> 
> --
> Sincerely yours,
> Mike.