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

[David Hildenbrand <david@redhat.com>]

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

-- 
Cheers

David / dhildenb