Re: [PATCH v2 04/12] mm: hugetlb_vmemmap: generalize vmemmap page allocation

[Muchun Song]

> On Nov 3, 2026, at 01:57, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> 
> On Mon, Nov 02, 2026 at 08:34:03PM +0800, Muchun Song wrote:
>>> On Nov 2, 2026, at 18:18, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
>>> 
>>> Hi Muchun,
>> 
>> Hi,
>> 
>> It's my email client's fault. Let me email it again.
> 
> Things happen :)
> 
>>>> Vmemmap page allocation is currently tied to restoring a virtual range
>>>> and assumes an empty destination list.  These constraints prevent it from
>>>> being reused when pages are requested independently of a vmemmap range or
>>>> appended to an existing list.
>>>> 
>>>> Generalize the allocator and give it an all-or-nothing list contract so
>>>> the restore and optimization paths can share the same allocation policy.
>>>> This also allows HVO head-page allocation to fail gracefully under memory
>>>> pressure without triggering the OOM killer.
>>>> 
>>>> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>>>> Reviewed-by: Lance Yang <lance.yang@xxxxxxxxx>
>>>> Reviewed-by: Qi Zheng <qi.zheng@xxxxxxxxx>
>>>> ---
>>>> v2:
>>>> - Collect Reviewed-by from Lance Yang and Qi Zheng
>>>> ---
>>>> mm/hugetlb_vmemmap.c | 39 ++++++++++++++++++---------------------
>>>> 1 file changed, 18 insertions(+), 21 deletions(-)
>>>> 
>>>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>>>> index 0e5b2b182d1bf..4b20a8efc0678 100644
>>>> --- a/mm/hugetlb_vmemmap.c
>>>> +++ b/mm/hugetlb_vmemmap.c
>>>> @@ -334,28 +334,26 @@ static int vmemmap_remap_free(unsigned long start, unsigned long end,
>>>> 	return ret;
>>>> }
>>>> 
>>>> -static int alloc_vmemmap_page_list(unsigned long start, unsigned long end,
>>>> -				    struct list_head *list)
>>>> +static int alloc_vmemmap_page_list(struct list_head *head, unsigned int nr_pages,
>>>> +				    int nid)
>>> 
>>> Please indent with two tabs, like David likes :)
>>> Applies throughout the series.
>> 
>> Sure. Should we update scripts/checkpatch.pl in the future for MM,
>> since this script seems to favor the alignment approach?
> 
> Yes, makes sense to me. 
> 
>>>> -	for (i = 0; i < nr_pages; i++) {
>>>> -		page = alloc_pages_node(nid, gfp_mask, 0);
>>>> -		if (!page)
>>>> -			goto out;
>>>> -		list_add(&page->lru, list);
>>>> +	for (unsigned int i = 0; i < nr_pages; i++) {
>>>> +		struct page *page;
>>>> +
>>>> +		page = alloc_pages_node(nid, GFP_KERNEL | __GFP_RETRY_MAYFAIL, 0);
>>>> +		if (!page) {
>>> 
>>> I think goto err_do_cleanup is cleaner than ad-hok cleanups
>> 
>> I’ll use goto style, either is OK for me.
>> 
>>> 
>>>> +			memmap_pages_add(i);
>>> 
>>> This is confusing. Updating counters only to use
>>> free_vmemmap_page_list() seems worse choice than walking the list and
>>> __free_page().
>> 
>> I think the memmap accounting should be done right after each successful
>> allocation — that way, using free_vmemmap_page_list() becomes more natural
>> and simple.
> 
> Why are you against using a loop with a plain __free_page()?

I'm not against. This is probably just my own fixation, I think I've fallen
too deeply into the trap of code that looks simple but actually isn't that
great. Good thing you pulled me out in time. So I'll use your suggested
approach to fix the problem here.

Thanks,
Muchun

> 
> We know for sure it's not a reserved page and there is no need in
> free_vmemmap_page().
> 
> And with memmap_pages_add() per page do a bunch of atomic_long_add()s
> instead of one.
> 
> BTW, it would have been nice to avoid per-page counters update in
> free_vmemmap_page_list() too, but that's not related to this series.
> 
>> Somthing like:
>> 
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index c04f53f708d2..cab9ec14873d 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
>> @@ -207,14 +207,13 @@ static int alloc_vmemmap_page_list(struct list_head *head, unsigned int nr_pages
>> 
>>                page = alloc_pages_node(nid, GFP_KERNEL | __GFP_RETRY_MAYFAIL, 0);
>>                if (!page) {
>> -                       memmap_pages_add(i);
>>                        free_vmemmap_page_list(&local);
>>                        return -ENOMEM;
>>                }
>> +               memmap_pages_add(1);
>>                list_add(&page->lru, &local);
>>        }
>>        list_splice(&local, head);
>> -       memmap_pages_add(nr_pages);
>> 
>>        return 0;
>> }
>> 
>> Thanks for your review, Mike.
>> 
>> Muchun
>> 
>>> 
>>> -- 
>>> Sincerely yours,
>>> Mike.
>> 
>> 
> 
> -- 
> Sincerely yours,
> Mike.