Re: [PATCH] mm: free surplus huge pages properly on NUMA systems

[Oscar Salvador <osalvador@suse.de>]

On Thu, May 15, 2025 at 10:13:27PM +0300, Andrey Alekhin wrote:
> == History ==
> 
> Wrong values of huge pages counters were detected on Red Hat 9.0 (linux
> 5.14) when runing ltp test hugemmap10. Inspection of linux source code
> showed that the problem was not fixed even in linux 6.14.

Besides code inspection, have you tried this on a current upstream
kernel? We have recently changed the way we deal with surplus huge
pages.

> == Problem ==
> 
> free_huge_folio function does not properly free surplus huge pages on
> NUMA systems. free_huge_folio checks surplus huge page counter only on
> current node (where folio is allocated), but gather_surplus_pages
> function can allocate surplus huge pages on any node.
> 
> The following sequence is possible on NUMA system:
> 
> n - overall number of huge pages
> f - number of free huge pages
> s - number of surplus huge pages
> huge page counters:  [before]
>                         |
>                      [after]
> 
> 		Process runs on node #1
> 			     |
>        node0               node1
> 1) addr1 = mmap(MAP_SHARED, ...) // 1 huge page is mmaped (cur_nid=1)
>    [n=2 f=2 s=0]       [n=1 f=1 s=0]  r=0
>                   |
>    [n=2 f=2 s=0]       [n=1 f=1 s=0]  r=1

I take that 'r' means reserved?

>  void free_huge_folio(struct folio *folio)
>  {
>  	/*
> @@ -1833,6 +1850,8 @@ void free_huge_folio(struct folio *folio)
>  	struct hugepage_subpool *spool = hugetlb_folio_subpool(folio);
>  	bool restore_reserve;
>  	unsigned long flags;
> +	int node;
> +	nodemask_t *mbind_nodemask, alloc_nodemask;
>  
>  	VM_BUG_ON_FOLIO(folio_ref_count(folio), folio);
>  	VM_BUG_ON_FOLIO(folio_mapcount(folio), folio);
> @@ -1883,6 +1902,25 @@ void free_huge_folio(struct folio *folio)
>  		remove_hugetlb_folio(h, folio, true);
>  		spin_unlock_irqrestore(&hugetlb_lock, flags);
>  		update_and_free_hugetlb_folio(h, folio, true);
> +	} else if (h->surplus_huge_pages) {
> +		mbind_nodemask = policy_mbind_nodemask(htlb_alloc_mask(h));
> +		if (mbind_nodemask)
> +			nodes_and(alloc_nodemask, *mbind_nodemask,
> +					cpuset_current_mems_allowed);
> +		else
> +			alloc_nodemask = cpuset_current_mems_allowed;
> +
> +		for_each_node_mask(node, alloc_nodemask) {
> +			if (h->surplus_huge_pages_node[node]) {
> +				h->surplus_huge_pages_node[node]--;
> +				h->surplus_huge_pages--;
> +				break;
> +			}
> +		}

I am not really happy with this, it feels quite ad-hoc to be honest.
If we are really having this, and I need to take closer look, we should join
the two 'else if' that handle surplus pages because as it stands it is
not really obvious what is going on.
And we might need some comments as well.

Let me think about this.


-- 
Oscar Salvador
SUSE Labs