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

[Hongfu Li]

在 11/10/26 20:45, 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.

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