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

[Muchun Song <muchun.song@linux.dev>]

> On Apr 13, 2026, at 22:05, Muchun Song <muchun.song@linux.dev> wrote:
> 
> 
> 
>> 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().

To follow  "all or nothing" principle here, I think we should not call
section_deactivate() to do the cleanup in section_activate().

After all, if section_activate() didn't succeed, how can we use
section_deactivate() to release the resources? What do you think?

Thanks.

>> 
>>> Thanks,
>>> Muchun.
>> 
>> --
>> Sincerely yours,
>> Mike.