Re: [PATCH v2 4/4] mm: hugetlb: Avoid re-allocating global reservations on region add failure

[Ackerley Tng]

Joshua Hahn <joshua.hahnjy@xxxxxxxxx> writes:

> On Wed, 09 Sep 2026 14:49:29 -0700 Ackerley Tng via B4 Relay <devnull+ackerleytng.google.com@xxxxxxxxxx> wrote:
>
>> From: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>>
>> When reserving huge pages for a shared mapping, reservations are first
>> requested from the subpool, and any remainder is accounted in global
>> reservations. When adding the file region entries fails later in the
>> process, the reservation attempt must be rolled back.
>>
>> Previously, this error path explicitly dropped the global reservations
>> that were just acquired before jumping to the cleanup label. The cleanup
>> label then returned the pages to the subpool. If concurrent activity in
>> the subpool allowed the subpool to absorb more reservations upon return
>> than it supplied initially, the cleanup label calculated a positive
>> difference and attempted to allocate new global reservations from scratch.
>>
>> This premature release was completely unnecessary because all requested
>> pages were already backed globally: partly by the mount guarantee and
>> partly by the global reservations just acquired. Prematurely dissolving
>> those reservations forced the cleanup path to attempt fresh buddy
>> allocations that could fail under memory pressure.
>>
>> Instead, track the number of global reservations actually accounted so
>> far. In the cleanup label, subtract the already-accounted amount from the
>> difference between requested and returned reservations. This ensures
>> that when global reservations were already acquired, the adjustment is
>> purely non-positive, dropping excess reservations without ever attempting
>> fresh allocations.
>>
>> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>> ---
>>  mm/hugetlb.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 652cfb55c6e6e..1151ad959ffd5 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -6678,6 +6678,7 @@ long hugetlb_reserve_pages(struct inode *inode,
>>  	struct hugepage_subpool *spool = subpool_inode(inode);
>>  	struct resv_map *resv_map;
>>  	struct hugetlb_cgroup *h_cg = NULL;
>> +	long gbl_resv_accted = 0;
>
> Sorry, I think I have a hard time with this variable name ;p (I know,
> even though the function is called huetlb_acct_memory) I get a little
> bit confused because I can't tell if it's meant to be "accepted"
> or "accounted".
>

Haha, didn't make the link between accted and accepted! In the next
revision I'll use gbl_resv_accounted instead.

> I know it puts the line below at 81 columns :p but maybe we can just
> split it across 2 lines?
>

I think this counts as an exception (more readable) so 1 line is okay?
Either way line wrapping shouldn't be a reason for using accted. I just
didn't make the connection between accted and accepted.

Thanks for reviewing!

>>  	long regions_needed = 0;
>>  	long gbl_resv_get;
>>  	long gbl_resv_put;
>> @@ -6768,6 +6769,7 @@ long hugetlb_reserve_pages(struct inode *inode,
>>  	err = hugetlb_acct_memory(h, gbl_resv_get);
>>  	if (err < 0)
>>  		goto out_put_pages;
>> +	gbl_resv_accted = gbl_resv_get;
>>
>>  	/*
>>  	 * Account for the reservations made. Shared mappings record regions
>> @@ -6784,7 +6786,6 @@ long hugetlb_reserve_pages(struct inode *inode,
>>  		add = region_add(resv_map, from, to, regions_needed, h, h_cg);
>>
>>  		if (unlikely(add < 0)) {
>> -			hugetlb_acct_memory(h, -gbl_resv_get);
>>  			err = add;
>>  			goto out_put_pages;
>>  		} else if (unlikely(chg > add)) {
>> @@ -6831,9 +6832,10 @@ long hugetlb_reserve_pages(struct inode *inode,
>>  	 * There may be a difference between the number of
>>  	 * reservations to consume and the number to restore now if
>>  	 * there are multiple threads interacting with the subpool -
>> -	 * restore the difference.
>> +	 * restore the difference, taking into account any global
>> +	 * reservations already acquired.
>>  	 */
>> -	hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put);
>> +	hugetlb_acct_memory(h, gbl_resv_get - gbl_resv_put - gbl_resv_accted);
>>
>>  out_uncharge_cgroup:
>>  	hugetlb_cgroup_uncharge_cgroup_rsvd(hstate_index(h),
>>
>> --
>> 2.55.0.1007.g17ff1f9808-goog