Re: [PATCH v3] mm/hugetlb: fix max-only subpool accounting on alloc_hugetlb_folio failure

[David Hildenbrand (Arm)]

On 10/13/26 17:04, David Hildenbrand (Arm) wrote:
> On 4/28/26 13:30, Zhao Li wrote:
>> alloc_hugetlb_folio() calls hugepage_subpool_get_pages() when map_chg
>> is set.  For a subpool with max_hpages != -1, that bumps used_hpages
>> regardless of whether it returns gbl_chg = 0 (rsv slot consumed) or
>> gbl_chg > 0 (used_hpages slot only).  If the allocation later fails
>> before a folio is returned, the unwind must undo the used_hpages
>> bump.  The old cleanup only ran for !gbl_chg, leaking used_hpages on
>> the gbl_chg > 0 path.
>>
>> For gbl_chg > 0 on max-only subpools (max_hpages != -1, min_hpages
>> == -1), hugepage_subpool_get_pages() took only a speculative
>> used_hpages slot.  Drop that slot directly under spool->lock.  In
>> that configuration hugepage_subpool_put_pages() cannot restore
>> rsv_hpages, so the direct decrement is the exact inverse and is
>> race-free against concurrent puts.  This matches the used_hpages-only
>> part of hugetlb_reserve_pages()'s out_put_pages cleanup, but
>> restricts it to the max-only case where no rsv_hpages restoration is
>> possible.
>>
>> Mounts with min_hpages != -1 are left unchanged for now.  v2's
>> approach (hugepage_subpool_put_pages() + h->resv_huge_pages++ to
>> back a restored rsv_hpages slot) double-counts global backing under
>> concurrent free_huge_folio() and creates phantom reservations under
>> concurrent hugetlb_unreserve_pages().  Safe cleanup of that quadrant
>> needs a coordinated fix across multiple call sites.
>>
>> Reproduced on size=20M hugetlbfs with the faulting task in a hugetlb
>> cgroup whose limit is exceeded.  Vanilla leaks 6/8 hugepages of
>> subpool quota; this patch leaks 0/8.  Verified under QEMU.
>>
>> Fixes: a833a693a490 ("mm: hugetlb: fix incorrect fallback for subpool")
>> Cc: stable@xxxxxxxxxxxxxxx # v6.15+
>> Signed-off-by: Zhao Li <enderaoelyther@xxxxxxxxx>
>> ---
>> Changes in v3:
>> - Replace v2's hugepage_subpool_put_pages() + h->resv_huge_pages++ on
>>   the gbl_chg > 0 branch with a direct used_hpages-- under spool->lock.
>> - Restrict the cleanup to (max_hpages != -1, min_hpages == -1) where
>>   the direct decrement is the exact inverse of the speculative bump.
>>
>> Changes in v2:
>> - Skip the gbl_chg > 0 cleanup when max_hpages is unset.
>> - Add hugepage_subpool_put_pages() + h->resv_huge_pages++ on the
>>   gbl_chg > 0 branch.
>>
>>  mm/hugetlb.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index f24bf49be047e..cfdeaf6394c5b 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -3025,13 +3025,24 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
>>  		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, pages_per_huge_page(h),
>>  						    h_cg);
>>  out_subpool_put:
>> -	/*
>> -	 * put page to subpool iff the quota of subpool's rsv_hpages is used
>> -	 * during hugepage_subpool_get_pages.
>> -	 */
>> -	if (map_chg && !gbl_chg) {
>> -		gbl_reserve = hugepage_subpool_put_pages(spool, 1);
>> -		hugetlb_acct_memory(h, -gbl_reserve);
>> +	if (map_chg) {
> 
> 
> I agree with Muchuns comment about removing the "gbl_chg > 0" check.
> 
> You could consider the following structure, which reduces indentation and avoids
> touching the existing handling.
> 
> if (map_chg && !gbl_chg) {
> 	...
> } else if (map_chg && spool && spool->min_hpages == -1 &&
> 	   spool->max_hpages != -1) {
> 	...
> }
> 
> But the code would read a lot nicer as
> 
> if (map_chg && !gbl_chg) {
> 	...
> } else if (map_chg) {
> 	hugepage_subpool_XXX_pages()
> }
> 
> Whereby the spool checks etc are handled in there.
> 
> Muchun things that we might just be able to use hugepage_subpool_put_pages(). It
> that's possible, great. Otherwise create a new helper that wraps all the logic
> below.
> 

Any update on this?

-- 
Cheers,

David