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

[Muchun Song]

> On Nov 4, 2026, at 17:58, Oscar Salvador (SUSE) <osalvador@xxxxxxxxxx> wrote:
> 
> On Wed, Nov 04, 2026 at 10:35:02AM +0800, Muchun Song wrote:
>> 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>
> 
> Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>

Thanks for your review.

> 
> A small nit below:
> 
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index 0e5b2b182d1b..d3058e106795 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
> ...
>> +static int alloc_vmemmap_page_list(struct list_head *head,
>> +		unsigned int nr_pages, int nid)
>> {
>> -	gfp_t gfp_mask = GFP_KERNEL | __GFP_RETRY_MAYFAIL;
>> -	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
>> -	int nid = page_to_nid((struct page *)start);
>> +	LIST_HEAD(new_pages);
>> 	struct page *page, *next;
>> -	int i;
>> 
>> -	for (i = 0; i < nr_pages; i++) {
>> -		page = alloc_pages_node(nid, gfp_mask, 0);
>> +	for (unsigned int i = 0; i < nr_pages; i++) {
>> +		page = alloc_pages_node(nid, GFP_KERNEL | __GFP_RETRY_MAYFAIL, 0);
>> 		if (!page)
>> -			goto out;
>> -		list_add(&page->lru, list);
>> +		goto free_pages;
>> +		list_add(&page->lru, &new_pages);
>> }
>> +	list_splice(&new_pages, head);
>> 	memmap_pages_add(nr_pages);
>> 
>> 	return 0;
>> -out:
>> -	list_for_each_entry_safe(page, next, list, lru)
>> +free_pages:
>> +	list_for_each_entry_safe(page, next, &new_pages, lru)
>> 		__free_page(page);
>> 	return -ENOMEM;
>> }
> ...
>> @@ -527,15 +526,13 @@ static int __hugetlb_vmemmap_optimize_folio(const struct hstate *h,
>>  */
>> folio_set_hugetlb_vmemmap_optimized(folio);
>> 
>> -	vmemmap_head = alloc_pages_node(nid, GFP_KERNEL, 0);
>> -	if (!vmemmap_head) {
>> +	if (alloc_vmemmap_page_list(vmemmap_pages, VMEMMAP_OPTIMIZATION_PAGES, nid)) {
>> 		ret = -ENOMEM;
>> 		goto out;
> 
> alloc_vmemmap_page_list() already gives you the error code (ENOMEM), so
> it would look more natural to me to use that already and not "overwrite"
> it.
> e.g: 
> 
> ret = alloc_vmemmap_page_list
> if (ret)
>     goto out;

Agreed, thanks — I'll just propagate the return value next version.

Muchun

> 
> but as I said, just a nit :-)
> 
> 
> -- 
> Oscar Salvador
> SUSE Labs