Re: [PATCH v3 3/6] mm: page_alloc: add alloc_contig_{range_frozen,frozen_pages}()

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

+ Matthew

On 2025/10/20 21:07, David Hildenbrand wrote:
> 
>>>
>>>> +void free_contig_range_frozen(unsigned long pfn, unsigned long 
>>>> nr_pages)
>>>> +{
>>>> +    struct folio *folio = pfn_folio(pfn);
>>>> +
>>>> +    if (folio_test_large(folio)) {
>>>> +        int expected = folio_nr_pages(folio);
>>>> +
>>>> +        WARN_ON(folio_ref_count(folio));
>>>> +
>>>> +        if (nr_pages == expected)
>>>> +            free_frozen_pages(&folio->page, folio_order(folio));
>>>> +        else
>>>> +            WARN(true, "PFN %lu: nr_pages %lu != expected %d\n",
>>>> +                 pfn, nr_pages, expected);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    for (; nr_pages--; pfn++) {
>>>> +        struct page *page = pfn_to_page(pfn);
>>>> +
>>>> +        WARN_ON(page_ref_count(page));
>>>> +        free_frozen_pages(page, 0);
>>>> +    }
>>>
>>> That's mostly a copy-and-paste of free_contig_range().
>>>
>>> I wonder if there is some way to avoid duplicating a lot of
>>> free_contig_range() here. Hmmm.
>>>
>>> Also, the folio stuff in there looks a bit weird I'm afraid.
>>>
>>> Can't we just refuse to free compound pages throught this interface and
>>> free_contig_range() ? IIRC only hugetlb uses it and uses folio_put()
>>> either way?
>>>
>>> Then we can just document that compound allocations are to be freed
>>> differently.
>>
>>
>> There is a case for cma_free_folio, which calls free_contig_range for
>> both in cma_release(), but I will try to check whether we could avoid
>> the folio stuff in free_contig_range().
> 
> 
> Ah, right, there is hugetlb_cma_free_folio()->cma_free_folio().
> 
> And we need that, because we have to make sure that CMA stats are 
> updated properly.
> 
> All compound page handling in the freeing path is just nasty and not 
> particularly future-proof regarding memdescs.
> 
> I wonder if we could just teach alloc_contig to never hand out compound 
> pages and then let the freeing path similarly assert that there are no 
> compound pages.
> 
> Whoever wants a compound page (currently only hugetlb?) can create that 
> from a frozen range. Before returning the frozen range the compound page 
> can be dissolved. That way also any memdesc can be allocated/freed by 
> the caller later.
> 
> The only nasty thing is the handing of splitting/merging of 
> set_page_owner/page_table_check_alloc etc. :(
> 
> 
> 
> As an alternative, we could only allow compound pages for frozen pages. 
> This way, we'd force any caller to handle the allocation/freeing of the 
> memdesc in the future manually.
> 
> Essentially, only allow GFP_COMPOUND on the frozen interface, which we 
> would convert hugetlb to.
> 
> That means that we can simplify free_contig_range() [no need to handle 
> compound pages]. For free_contig_frozen_range() we would skip refcount 
> checks on that level and do something like:
> 
> void free_contig_frozen_range(unsigned long pfn, unsigned long nr_pages)
> {
>      struct page *first_page = pfn_to_page(pfn)
>      const unsigned int order = ilog2(nr_pages);
> 
>      if (PageHead(first_page)) {
>          WARN_ON_ONCE(order != compound_order(first_page));
>          free_frozen_pages(first_page, order);
>          return;
>      }
> 
>      for (; nr_pages--; pfn++)
>          free_frozen_pages(pfn_to_page(pfn), 0);
> }
> 
> CCing Willy, I don't know yet what will be better in the future. But the 
> folio stuff in there screams for problems.
> 

Sorry forget to add cc in v3, the full link[1],

[1] 
https://lore.kernel.org/linux-mm/20251013133854.2466530-1-wangkefeng.wang@huawei.com/