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

[Gregory Price (Meta)]

On 2026-11-10 14:05 +0800, Hongfu Li wrote:
> From: Hongfu Li <lihongfu@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Hongfu Li <lihongfu@xxxxxxxxxx>

Reviewed-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
Tested-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>

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.

~Gregory

-- 
~Gregory