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

[Muchun Song]

> On Apr 13, 2026, at 21:36, Mike Rapoport <rppt@xxxxxxxxxx> 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@xxxxxxxxxx> 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.