Message-ID: <diqzsek3mh22.fsf@ackerleytng-ctop.c.googlers.com>
Date: Fri, 13 Jun 2025 15:07:33 -0700
From: Ackerley Tng <ackerleytng@...gle.com>
To: Ira Weiny <ira.weiny@...el.com>
Cc: kvm@...r.kernel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	x86@...nel.org, linux-fsdevel@...r.kernel.org, aik@....com, 
	ajones@...tanamicro.com, akpm@...ux-foundation.org, amoorthy@...gle.com, 
	anthony.yznaga@...cle.com, anup@...infault.org, aou@...s.berkeley.edu, 
	bfoster@...hat.com, binbin.wu@...ux.intel.com, brauner@...nel.org, 
	catalin.marinas@....com, chao.p.peng@...el.com, chenhuacai@...nel.org, 
	dave.hansen@...el.com, david@...hat.com, dmatlack@...gle.com, 
	dwmw@...zon.co.uk, erdemaktas@...gle.com, fan.du@...el.com, fvdl@...gle.com, 
	graf@...zon.com, haibo1.xu@...el.com, hch@...radead.org, hughd@...gle.com, 
	ira.weiny@...el.com, isaku.yamahata@...el.com, jack@...e.cz, 
	james.morse@....com, jarkko@...nel.org, jgg@...pe.ca, jgowans@...zon.com, 
	jhubbard@...dia.com, jroedel@...e.de, jthoughton@...gle.com, 
	jun.miao@...el.com, kai.huang@...el.com, keirf@...gle.com, 
	kent.overstreet@...ux.dev, kirill.shutemov@...el.com, liam.merwick@...cle.com, 
	maciej.wieczor-retman@...el.com, mail@...iej.szmigiero.name, maz@...nel.org, 
	mic@...ikod.net, michael.roth@....com, mpe@...erman.id.au, 
	muchun.song@...ux.dev, nikunj@....com, nsaenz@...zon.es, 
	oliver.upton@...ux.dev, palmer@...belt.com, pankaj.gupta@....com, 
	paul.walmsley@...ive.com, pbonzini@...hat.com, pdurrant@...zon.co.uk, 
	peterx@...hat.com, pgonda@...gle.com, pvorel@...e.cz, qperret@...gle.com, 
	quic_cvanscha@...cinc.com, quic_eberman@...cinc.com, 
	quic_mnalajal@...cinc.com, quic_pderrin@...cinc.com, quic_pheragu@...cinc.com, 
	quic_svaddagi@...cinc.com, quic_tsoni@...cinc.com, richard.weiyang@...il.com, 
	rick.p.edgecombe@...el.com, rientjes@...gle.com, roypat@...zon.co.uk, 
	rppt@...nel.org, seanjc@...gle.com, shuah@...nel.org, steven.price@....com, 
	steven.sistare@...cle.com, suzuki.poulose@....com, tabba@...gle.com, 
	thomas.lendacky@....com, usama.arif@...edance.com, vannapurve@...gle.com, 
	vbabka@...e.cz, viro@...iv.linux.org.uk, vkuznets@...hat.com, 
	wei.w.wang@...el.com, will@...nel.org, willy@...radead.org, 
	xiaoyao.li@...el.com, yan.y.zhao@...el.com, yilun.xu@...el.com, 
	yuzenghui@...wei.com, zhiquan1.li@...el.com
Subject: Re: [RFC PATCH v2 23/51] mm: hugetlb: Refactor out hugetlb_alloc_folio()

Ira Weiny <ira.weiny@...el.com> writes:

> Ackerley Tng wrote:
>> Refactor out hugetlb_alloc_folio() from alloc_hugetlb_folio(), which
>> handles allocation of a folio and cgroup charging.
>>
>> Other than flags to control charging in the allocation process,
>> hugetlb_alloc_folio() also has parameters for memory policy.
>>
>> This refactoring as a whole decouples the hugetlb page allocation from
>> hugetlbfs, (1) where the subpool is stored at the fs mount, (2)
>> reservations are made during mmap and stored in the vma, and (3) mpol
>> must be stored at vma->vm_policy (4) a vma must be used for allocation
>> even if the pages are not meant to be used by host process.
>>
>> This decoupling will allow hugetlb_alloc_folio() to be used by
>> guest_memfd in later patches. In guest_memfd, (1) a subpool is created
>> per-fd and is stored on the inode, (2) no vma-related reservations are
>> used (3) mpol may not be associated with a vma since (4) for private
>> pages, the pages will not be mappable to userspace and hence have to
>> associated vmas.
>>
>> This could hopefully also open hugetlb up as a more generic source of
>> hugetlb pages that are not bound to hugetlbfs, with the complexities
>> of userspace/mmap/vma-related reservations contained just to
>> hugetlbfs.
>>
>> Signed-off-by: Ackerley Tng <ackerleytng@...gle.com>
>> Change-Id: I60528f246341268acbf0ed5de7752ae2cacbef93
>> ---
>>  include/linux/hugetlb.h |  12 +++
>>  mm/hugetlb.c            | 192 ++++++++++++++++++++++------------------
>>  2 files changed, 118 insertions(+), 86 deletions(-)
>>
>
> [snip]
>
>>
>> +/**
>> + * hugetlb_alloc_folio() - Allocates a hugetlb folio.
>> + *
>> + * @h: struct hstate to allocate from.
>> + * @mpol: struct mempolicy to apply for this folio allocation.
>> + * @ilx: Interleave index for interpretation of @mpol.
>> + * @charge_cgroup_rsvd: Set to true to charge cgroup reservation.
>> + * @use_existing_reservation: Set to true if this allocation should use an
>> + *                            existing hstate reservation.
>> + *
>> + * This function handles cgroup and global hstate reservations. VMA-related
>> + * reservations and subpool debiting must be handled by the caller if necessary.
>> + *
>> + * Return: folio on success or negated error otherwise.
>> + */
>> +struct folio *hugetlb_alloc_folio(struct hstate *h, struct mempolicy *mpol,
>> +				  pgoff_t ilx, bool charge_cgroup_rsvd,
>> +				  bool use_existing_reservation)
>> +{
>> +	unsigned int nr_pages = pages_per_huge_page(h);
>> +	struct hugetlb_cgroup *h_cg = NULL;
>> +	struct folio *folio = NULL;
>> +	nodemask_t *nodemask;
>> +	gfp_t gfp_mask;
>> +	int nid;
>> +	int idx;
>> +	int ret;
>> +
>> +	idx = hstate_index(h);
>> +
>> +	if (charge_cgroup_rsvd) {
>> +		if (hugetlb_cgroup_charge_cgroup_rsvd(idx, nr_pages, &h_cg))
>> +			goto out;
>
> Why not just return here?
> 			return ERR_PTR(-ENOSPC);
>

I wanted to consistently exit the function on errors at the same place,
and also make this refactoring look like I just took the middle of
alloc_hugetlb_folio() out as much as possible.

>> +	}
>> +
>> +	if (hugetlb_cgroup_charge_cgroup(idx, nr_pages, &h_cg))
>> +		goto out_uncharge_cgroup_reservation;
>> +
>> +	gfp_mask = htlb_alloc_mask(h);
>> +	nid = policy_node_nodemask(mpol, gfp_mask, ilx, &nodemask);
>> +
>> +	spin_lock_irq(&hugetlb_lock);
>> +
>> +	if (use_existing_reservation || available_huge_pages(h))
>> +		folio = dequeue_hugetlb_folio(h, gfp_mask, mpol, nid, nodemask);
>> +
>> +	if (!folio) {
>> +		spin_unlock_irq(&hugetlb_lock);
>> +		folio = alloc_surplus_hugetlb_folio(h, gfp_mask, mpol, nid, nodemask);
>> +		if (!folio)
>> +			goto out_uncharge_cgroup;
>> +		spin_lock_irq(&hugetlb_lock);
>> +		list_add(&folio->lru, &h->hugepage_activelist);
>> +		folio_ref_unfreeze(folio, 1);
>> +		/* Fall through */
>> +	}
>> +
>> +	if (use_existing_reservation) {
>> +		folio_set_hugetlb_restore_reserve(folio);
>> +		h->resv_huge_pages--;
>> +	}
>> +
>> +	hugetlb_cgroup_commit_charge(idx, nr_pages, h_cg, folio);
>> +
>> +	if (charge_cgroup_rsvd)
>> +		hugetlb_cgroup_commit_charge_rsvd(idx, nr_pages, h_cg, folio);
>> +
>> +	spin_unlock_irq(&hugetlb_lock);
>> +
>> +	gfp_mask = htlb_alloc_mask(h) | __GFP_RETRY_MAYFAIL;
>> +	ret = mem_cgroup_charge_hugetlb(folio, gfp_mask);
>> +	/*
>> +	 * Unconditionally increment NR_HUGETLB here. If it turns out that
>> +	 * mem_cgroup_charge_hugetlb failed, then immediately free the page and
>> +	 * decrement NR_HUGETLB.
>> +	 */
>> +	lruvec_stat_mod_folio(folio, NR_HUGETLB, pages_per_huge_page(h));
>> +
>> +	if (ret == -ENOMEM) {
>> +		free_huge_folio(folio);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	return folio;
>> +
>> +out_uncharge_cgroup:
>> +	hugetlb_cgroup_uncharge_cgroup(idx, nr_pages, h_cg);
>> +out_uncharge_cgroup_reservation:
>> +	if (charge_cgroup_rsvd)
>> +		hugetlb_cgroup_uncharge_cgroup_rsvd(idx, nr_pages, h_cg);
>
> I find the direct copy of the unwind logic from alloc_hugetlb_folio()
> cumbersome and it seems like a good opportunity to clean it up.
>

I really wanted to make this refactoring look like I just took the
middle of alloc_hugetlb_folio() out as much as possible, to make it
obvious and understandable. I think the cleanup can be a separate patch
(series?)

>> +out:
>> +	folio = ERR_PTR(-ENOSPC);
>> +	goto out;
>
> Endless loop?
>

Thanks, this should have been

return folio;

> Ira
>
> [snip]

Powered by blists - more mailing lists