Re: [PATCH v2] mm/hugetlb: charge hugetlb cgroup and memcg in alloc_hugetlb_folio_reserve()

[Hongfu Li <hongfu.li@linux.dev>]

在 11/10/26 20:45, Gregory Price (Meta) 写道:
> On 2026-11-10 14:05 +0800, Hongfu Li wrote:
>> From: Hongfu Li <lihongfu@kylinos.cn>
>>
>> alloc_hugetlb_folio_reserve() hands out a folio from the hugetlb pool and
>> consumes a global reservation, but unlike the fault path it never charged
>> the folio to the hugetlb cgroup or to the memcg.  Folios allocated for
>> memfd_pin_folios() through memfd_alloc_folio() were therefore silently
>> bypassing the accounting: they are never charged to the hugetlb cgroup
>> usage counter (hugetlb.<size>.current or hugetlb.<size>.usage_in_bytes),
>> so the limits (hugetlb.<size>.max or hugetlb.<size>.limit_in_bytes) are
>> bypassed.  In cgroup v2, with memory_hugetlb_accounting enabled, these
>> folios are also not accounted in memory.current and are not bounded by
>> memory.max.
>>
>> Charge the folios to both the hugetlb cgroup and the memcg, just like
>> hugetlb_alloc_folio() does.
>>
>> Test with a process that creates a hugetlb memfd and pins pages through
>> UDMABUF_CREATE. The counters were read while the folio was still pinned:
>>
> ...
>> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning memfd folios")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>
> Reviewed-by: Gregory Price (Meta) <gourry@gourry.net>
> Tested-by: Gregory Price (Meta) <gourry@gourry.net>
>
> one note below that is worth assessing but maybe not a blocker.
>
>> +		ret = mem_cgroup_charge_hugetlb(folio, current->mm,
>> +						gfp_mask | __GFP_RETRY_MAYFAIL);
>> +		if (ret == -ENOMEM) {
>> +			folio_put(folio);
>> +			/*
>> +			 * Skip uncharging hugetlb_cgroup since the charges
>> +			 * were committed to the folio and freeing the folio
>> +			 * would have cleared those up.
>> +			 */
>> +			return NULL;
>> +		}
> This adds a new -ENOMEM condition on stable which may race subtly on
> two tasks allocating against the same memory.max and cause failures.
> In testing, it doesn't cause splats or anything, it may just cause
> spurious allocation failures that fallback to other paths.
>
> This implication may be worth a comment in the changelog.

Hi Gregory,

Thanks for the review and the testing.

Agreed on the new -ENOMEM condition. It is a real behaviour change that
affects stable kernels. I will add the following to the commit message
in v3:

memfd_pin_folios() can now fail because the memcg charge fails. Two
tasks allocating concurrently against the same memory.max may see a
spurious failure. That failure frees the folio, returns the global
reservation, and undoes the hugetlb cgroup charge, so no accounting is
left behind.

>
> ~Gregory
>
-- 
Best regards,
Hongfu