[PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[Oscar Salvador]

alloc_buddy_hugetlb_folio() allocates a rmappable folio, then strips the
rmappable part and freezes it.
We can simplify all that by allocating frozen pages directly.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 mm/hugetlb.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index e3e6ac991b9c..83fa2b9f6fc4 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1950,7 +1950,6 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h,
 	int order = huge_page_order(h);
 	struct folio *folio;
 	bool alloc_try_hard = true;
-	bool retry = true;
 
 	/*
 	 * By default we always try hard to allocate the folio with
@@ -1965,22 +1964,8 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h,
 		gfp_mask |= __GFP_RETRY_MAYFAIL;
 	if (nid == NUMA_NO_NODE)
 		nid = numa_mem_id();
-retry:
-	folio = __folio_alloc(gfp_mask, order, nid, nmask);
-	/* Ensure hugetlb folio won't have large_rmappable flag set. */
-	if (folio)
-		folio_clear_large_rmappable(folio);
 
-	if (folio && !folio_ref_freeze(folio, 1)) {
-		folio_put(folio);
-		if (retry) {	/* retry once */
-			retry = false;
-			goto retry;
-		}
-		/* WOW!  twice in a row. */
-		pr_warn("HugeTLB unexpected inflated folio ref count\n");
-		folio = NULL;
-	}
+	folio = (struct folio *)__alloc_frozen_pages(gfp_mask, order, nid, nmask);
 
 	/*
 	 * If we did not specify __GFP_RETRY_MAYFAIL, but still got a
-- 
2.49.0

Re: [PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[Vlastimil Babka]

On 4/11/25 15:23, Oscar Salvador wrote:
> alloc_buddy_hugetlb_folio() allocates a rmappable folio, then strips the
> rmappable part and freezes it.
> We can simplify all that by allocating frozen pages directly.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>

Much nice.

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Re: [PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[David Hildenbrand]

On 11.04.25 15:23, Oscar Salvador wrote:
> alloc_buddy_hugetlb_folio() allocates a rmappable folio, then strips the
> rmappable part and freezes it.
> We can simplify all that by allocating frozen pages directly.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>   mm/hugetlb.c | 17 +----------------
>   1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index e3e6ac991b9c..83fa2b9f6fc4 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1950,7 +1950,6 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h,
>   	int order = huge_page_order(h);
>   	struct folio *folio;
>   	bool alloc_try_hard = true;
> -	bool retry = true;
>   
>   	/*
>   	 * By default we always try hard to allocate the folio with
> @@ -1965,22 +1964,8 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h,
>   		gfp_mask |= __GFP_RETRY_MAYFAIL;
>   	if (nid == NUMA_NO_NODE)
>   		nid = numa_mem_id();
> -retry:
> -	folio = __folio_alloc(gfp_mask, order, nid, nmask);
> -	/* Ensure hugetlb folio won't have large_rmappable flag set. */
> -	if (folio)
> -		folio_clear_large_rmappable(folio);
>   
> -	if (folio && !folio_ref_freeze(folio, 1)) {
> -		folio_put(folio);
> -		if (retry) {	/* retry once */
> -			retry = false;
> -			goto retry;
> -		}
> -		/* WOW!  twice in a row. */
> -		pr_warn("HugeTLB unexpected inflated folio ref count\n");
> -		folio = NULL;
> -	}
> +	folio = (struct folio *)__alloc_frozen_pages(gfp_mask, order, nid, nmask);
>   
>   	/*
>   	 * If we did not specify __GFP_RETRY_MAYFAIL, but still got a

I assume htlb_alloc_mask() will always include _GFP_COMP.

But semantically, it might be wrong: __folio_alloc() will in the memdesc 
world also make sure to allocate the memdesc, __alloc_frozen_pages() not.

Maybe one would want a __alloc_frozen_folio() .... @willy?

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[Matthew Wilcox]

On Fri, Apr 11, 2025 at 03:44:31PM +0200, David Hildenbrand wrote:
> I assume htlb_alloc_mask() will always include _GFP_COMP.

static inline gfp_t htlb_alloc_mask(struct hstate *h)
{
        gfp_t gfp = __GFP_COMP | __GFP_NOWARN;

> But semantically, it might be wrong: __folio_alloc() will in the memdesc
> world also make sure to allocate the memdesc, __alloc_frozen_pages() not.
> 
> Maybe one would want a __alloc_frozen_folio() .... @willy?

This is fine.  Yes, it'll need to be modified when we get to the
separately allocated memdesc, but there's a number of places that
cast the freshly allocated page to a folio, and I'll have to come up
with a way to catch them all.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Oscar, if you want to take on the gigantic allocation next ...

 - I don't think we need folio_alloc_gigantic() to be wrapped in
   alloc_hooks
 - folio_alloc_gigantic() should return a frozen folio
 - as should hugetlb_cma_alloc_folio()

Re: [PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[David Hildenbrand]

On 11.04.25 16:21, Matthew Wilcox wrote:
> On Fri, Apr 11, 2025 at 03:44:31PM +0200, David Hildenbrand wrote:
>> I assume htlb_alloc_mask() will always include _GFP_COMP.
> 
> static inline gfp_t htlb_alloc_mask(struct hstate *h)
> {
>          gfp_t gfp = __GFP_COMP | __GFP_NOWARN;
> 

I checked that as well and actually meant to say the I assume that we 
will here always have __GFP_COMP because the gfp_mask we get here will 
always be derived from htlb_alloc_mask().

>> But semantically, it might be wrong: __folio_alloc() will in the memdesc
>> world also make sure to allocate the memdesc, __alloc_frozen_pages() not.
>>
>> Maybe one would want a __alloc_frozen_folio() .... @willy?
> 
> This is fine.  Yes, it'll need to be modified when we get to the
> separately allocated memdesc, but there's a number of places that
> cast the freshly allocated page to a folio, and I'll have to come up
> with a way to catch them all.

Okay, thanks for clarifying.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm,hugetlb: Allocate frozen pages in alloc_buddy_hugetlb_folio

[Oscar Salvador]

On Fri, Apr 11, 2025 at 03:21:42PM +0100, Matthew Wilcox wrote:
> Oscar, if you want to take on the gigantic allocation next ...
> 
>  - I don't think we need folio_alloc_gigantic() to be wrapped in
>    alloc_hooks
>  - folio_alloc_gigantic() should return a frozen folio
>  - as should hugetlb_cma_alloc_folio()

Yeah, I was taking a look at that as well.
I plan to tackle it in a few days.


-- 
Oscar Salvador
SUSE Labs