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

Muchun Song
2026-04-13T14:06:17Z

> On Apr 13, 2026, at 21:36, Mike Rapoport <rppt@kernel.org> wrote:
>
> ﻿Hi Muchun,

Hi,

>
> 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));

We don’t increase the counter on failure, then

> +
> + 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);

here section_deactivate() is called, which decrease the counter unconditionally,
the issue still exists. We didn't fix anything.

Thanks.

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