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

Muchun Song
2026-04-13T14:16:58Z

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