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

[David Hildenbrand <david@redhat.com>]

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

Yes, that would be very interesting.

> 
>>>
>>> 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);

I think it's weird that cma_alloc_frozen() consumes gfp_t when 
cma_alloc() doesn't. So I would wish that we can clean that up (-> 
remove gfp if possible).

So maybe we really want a

cma_alloc_frozen

and

cma_alloc_frozen_compound

whereby the latter consumes an "order" instead of "count + align".

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

Take a look at alloc_contig_range_noprof() where I added

	__alloc_contig_verify_gfp_mask()

Essentially, we ignore

	GFP_ZONEMASK | __GFP_RECLAIMABLE | __GFP_WRITE | __GFP_HARDWALL
        | __GFP_THISNODE | __GFP_MOVABLE

And we *always* set __GFP_RETRY_MAYFAIL

I'd argue that hugetlb passing in random parameters that don't really 
make sense for contig allcoations is rather an issue and makes you 
believe that they would have any effect.

If cma_alloc doesn't provide them a cma_alloc_frozen or cma_alloc_folio 
shouldn't provide them.

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

Yeah, that's probably a good first step, but while at it I would hope we 
could just cleanup the gfp stuff and provide a more consistent interface.

-- 
Cheers

David / dhildenb