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

[Mike Rapoport <rppt@kernel.org>]

On Mon, Nov 02, 2026 at 08:34:03PM +0800, Muchun Song wrote:
> > On Nov 2, 2026, at 18:18, Mike Rapoport <rppt@kernel.org> 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@bytedance.com>
> >> Reviewed-by: Lance Yang <lance.yang@linux.dev>
> >> Reviewed-by: Qi Zheng <qi.zheng@linux.dev>
> >> ---
> >> 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()?

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.