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

[David Hildenbrand <david@redhat.com>]

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
> 
> 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 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.

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().

-- 
Cheers

David / dhildenb