Re: [PATCH 4/4] mm: hugetlb: allocate frozen pages in alloc_gigantic_folio()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/9/12 15:23, David Hildenbrand wrote:
> On 12.09.25 09:18, David Hildenbrand wrote:
>> On 12.09.25 08:57, Kefeng Wang wrote:
>>>
>>>
>>> On 2025/9/12 2:56, David Hildenbrand wrote:
>>>> On 11.09.25 11:11, Kefeng Wang wrote:
>>>>>
>>>>>
>>>>> On 2025/9/11 16:25, David Hildenbrand wrote:
>>>>>> On 11.09.25 08:56, Kefeng Wang wrote:
>>>>>>> The alloc_gigantic_folio() allocates a folio by alloc_contig_range()
>>>>>>> with refcount increated and then freeze it, convert to allocate a
>>>>>>> frozen
>>>>>>> folio directly to remove the atomic operation about folio refcount,
>>>>>>> also
>>>>>>> saving atomic operation during __update_and_free_hugetlb_folio too.
>>>>>>>
>>>>>>> Rename some functions to make them more self-explanatory,
>>>>>>>
>>>>>>>       folio_alloc_gigantic            -> folio_alloc_frozen_gigantic
>>>>>>>       cma_{alloc,free}_folio          -> cma_{alloc,free} 
>>>>>>> _frozen_folio
>>>>>>>       hugetlb_cma_{alloc,free}_folio  -> hugetlb_cma_{alloc,free}
>>>>>>> _frozen_folio
>>>>>>
>>>>>> Can we just get rid of folio_alloc_frozen_gigantic?
>>>>>>
>>>>>
>>>>> OK, we could kill it.
>>>>>
>>>>>> Further, can we just get rid of cma_{alloc,free}_frozen_folio() as 
>>>>>> well
>>>>>> and just let hugetlb use alloc_contig_range_frozen() etc?
>>>>>
>>>>> HugeTLB can allocate folio by alloc_contig_frozen_pages() directly, 
>>>>> but
>>>>> it could allocate from hugetlb_cma, cma_alloc_folio() need change some
>>>>> cma metadata, so we need to keep it.
>>>>
>>>> Hm. Assuming we just have cma_alloc_frozen() -- again, probably what
>>>> cma_alloc() would look like in the future, hugetlb can just construct a
>>>> folio out of that.
>>>
>>> I get your point，firstly, we could convert to use cma_alloc_frozen()
>>> instead of cma_alloc_folio() in hugetlb_cma_alloc_folio().
>>>
>>>>
>>>> Maybe we just want a helper to create a folio out of a given page 
>>>> range?
>>>>
>>>> And that page range is either obtained through cma_alloc_frozen() or
>>>> alloc_contig_frozen_pages().
>>>>
>>>> Just a thought, keeping in mind that these things should probably just
>>>> work with frozen pages and let allcoating of a memdesc etc. be taken
>>>> care of someone else.
>>>>
>>>> I'd be happy if we can remove the GFP_COMPOUND parameter from
>>>> alloc_contig*.
>>>
>>> But not sure about this part,  GFP_COMPOUND for alloc_contig* is
>>> introduced by commit e98337d11bbd "mm/contig_alloc: support __GFP_COMP",
>>> if we still allocate a range of order-0 pages and create a folio
>>> outside, it will slow the large folio allocation.
>>
>> Assuming we leave the refcount untouched (frozen), I guess what's left is
>>
>> a) Calling post_alloc_hook() on each free buddy chunk we isolated
>> b) Splitting all pages to order 0
>>
>> Splitting is updating the page owner + alloc tag + memcg, and currently
>> still updating the refcount.
>>
>>
>> I would assume that most of the overhead came from the atomics when
>> updating the refcount in split_page, which we would optimize out.
>>
>>       Perf profile before:
>>         Alloc
>>           - 99.99% alloc_pool_huge_folio
>>              - __alloc_fresh_hugetlb_folio
>>                 - 83.23% alloc_contig_pages_noprof
>>                    - 47.46% alloc_contig_range_noprof
>>                       - 20.96% isolate_freepages_range
>>                            16.10% split_page
>>                       - 14.10% start_isolate_page_range
>>                       - 12.02% undo_isolate_page_range
>>
>> Would be interesting trying to see how much overhead would remain when
>> just dealing

Patch2 skip atomic update in split_page() with alloc_contig_frozen_pages(),
I could test performance about old alloc_contig_pages() with/without
GFP_COMP and new alloc_contig_frozen_pages() when allocate same size.

>>
>> OTOH, maybe we can leave GFP_COMPOUND support in but make the function
>> more generic, not limited to folios (I suspect many users will not want
>> folios, except hugetlb).
>>

Maybe just only add cma_alloc_frozen(), and 
cma_alloc()/hugetlb_cma_alloc_folio()
is the wrapper that calls it and set page refcount, like what we did in
other frozen allocation.

struct page *cma_alloc_frozen(struct cma *cma, unsigned long count,
				unsigned int align, gfp_t gfp);

>> Maybe just a
>>
>> struct page * cma_alloc_compound(struct cma *cma, unsigned int order,
>> unsigned int align, bool no_warn);
> 
> ^ no need for the align as I realized, just like
> cma_alloc_folio() doesn't have.

since cma_alloc_frozen is more generic, we need keep align,

> 
> I do wonder why we decided to allow cma_alloc_folio() to consume gfp_t 
> flags when we don't do the same for cma_alloc().
> 

cma_alloc_folio() now is called by hugetlb allocation, the gfp could be
htlb_alloc_mask(), with/without __GFP_THISNODE/__GFP_RETRY_MAYFAIL...
and this could be used in alloc_contig_frozen_pages(),(eg,
gfp_zone).

For cma_alloc() unconditional use GFP_KERNEL from commit
6518202970c1 "mm/cma: remove unsupported gfp_mask parameter from
cma_alloc()", but this is another story.

Back to this patchset, just add a new cma_alloc_frozen() shown
above and directly call it in cma_alloc()/hugetlb_cma_alloc_folio(),
get rid of folio_alloc_frozen_gigantic() and cma_alloc_folio(), and
we could do more optimization in the next step.