[PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[yangge1116]

From: Ge Yang <yangge1116@126.com>

In the isolate_or_dissolve_huge_folio() function, the folio_test_hugetlb()
function is called to determine whether a folio is a hugetlb folio.
However, in the subsequent alloc_and_dissolve_hugetlb_folio() function,
the folio_test_hugetlb() function is called again to make the same
determination about whether the folio is a hugetlb folio. It appears that
the folio_test_hugetlb() check in the isolate_or_dissolve_huge_folio()
function can be removed. Additionally, a similar issue exists in the
replace_free_hugepage_folios() function, and it should be addressed as
well.

Suggested-by: Oscar Salvador <osalvador@suse.de>
Signed-off-by: Ge Yang <yangge1116@126.com>
---
 mm/hugetlb.c | 51 +++++++++++++--------------------------------------
 1 file changed, 13 insertions(+), 38 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 6c2e007..6e46f2f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2787,20 +2787,24 @@ void restore_reserve_on_error(struct hstate *h, struct vm_area_struct *vma,
 /*
  * alloc_and_dissolve_hugetlb_folio - Allocate a new folio and dissolve
  * the old one
- * @h: struct hstate old page belongs to
  * @old_folio: Old folio to dissolve
  * @list: List to isolate the page in case we need to
  * Returns 0 on success, otherwise negated error.
  */
-static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
-			struct folio *old_folio, struct list_head *list)
+static int alloc_and_dissolve_hugetlb_folio(struct folio *old_folio,
+			struct list_head *list)
 {
-	gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
+	gfp_t gfp_mask;
+	struct hstate *h;
 	int nid = folio_nid(old_folio);
 	struct folio *new_folio = NULL;
 	int ret = 0;
 
 retry:
+	/*
+	 * The old_folio might have been dissolved from under our feet, so make sure
+	 * to carefully check the state under the lock.
+	 */
 	spin_lock_irq(&hugetlb_lock);
 	if (!folio_test_hugetlb(old_folio)) {
 		/*
@@ -2829,8 +2833,10 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
 		cond_resched();
 		goto retry;
 	} else {
+		h = folio_hstate(old_folio);
 		if (!new_folio) {
 			spin_unlock_irq(&hugetlb_lock);
+			gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
 			new_folio = alloc_buddy_hugetlb_folio(h, gfp_mask, nid,
 							      NULL, NULL);
 			if (!new_folio)
@@ -2874,35 +2880,20 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
 
 int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
 {
-	struct hstate *h;
 	int ret = -EBUSY;
 
 	/*
-	 * The page might have been dissolved from under our feet, so make sure
-	 * to carefully check the state under the lock.
-	 * Return success when racing as if we dissolved the page ourselves.
-	 */
-	spin_lock_irq(&hugetlb_lock);
-	if (folio_test_hugetlb(folio)) {
-		h = folio_hstate(folio);
-	} else {
-		spin_unlock_irq(&hugetlb_lock);
-		return 0;
-	}
-	spin_unlock_irq(&hugetlb_lock);
-
-	/*
 	 * Fence off gigantic pages as there is a cyclic dependency between
 	 * alloc_contig_range and them. Return -ENOMEM as this has the effect
 	 * of bailing out right away without further retrying.
 	 */
-	if (hstate_is_gigantic(h))
+	if (folio_order(folio) > MAX_PAGE_ORDER)
 		return -ENOMEM;
 
 	if (folio_ref_count(folio) && folio_isolate_hugetlb(folio, list))
 		ret = 0;
 	else if (!folio_ref_count(folio))
-		ret = alloc_and_dissolve_hugetlb_folio(h, folio, list);
+		ret = alloc_and_dissolve_hugetlb_folio(folio, list);
 
 	return ret;
 }
@@ -2916,7 +2907,6 @@ int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
  */
 int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
 {
-	struct hstate *h;
 	struct folio *folio;
 	int ret = 0;
 
@@ -2925,23 +2915,8 @@ int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
 	while (start_pfn < end_pfn) {
 		folio = pfn_folio(start_pfn);
 
-		/*
-		 * The folio might have been dissolved from under our feet, so make sure
-		 * to carefully check the state under the lock.
-		 */
-		spin_lock_irq(&hugetlb_lock);
-		if (folio_test_hugetlb(folio)) {
-			h = folio_hstate(folio);
-		} else {
-			spin_unlock_irq(&hugetlb_lock);
-			start_pfn++;
-			continue;
-		}
-		spin_unlock_irq(&hugetlb_lock);
-
 		if (!folio_ref_count(folio)) {
-			ret = alloc_and_dissolve_hugetlb_folio(h, folio,
-							       &isolate_list);
+			ret = alloc_and_dissolve_hugetlb_folio(folio, &isolate_list);
 			if (ret)
 				break;
 
-- 
2.7.4

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[Oscar Salvador]

On Fri, May 23, 2025 at 04:05:59PM +0800, yangge1116@126.com wrote:
> From: Ge Yang <yangge1116@126.com>
> 
> In the isolate_or_dissolve_huge_folio() function, the folio_test_hugetlb()
> function is called to determine whether a folio is a hugetlb folio.
> However, in the subsequent alloc_and_dissolve_hugetlb_folio() function,
> the folio_test_hugetlb() function is called again to make the same
> determination about whether the folio is a hugetlb folio. It appears that
> the folio_test_hugetlb() check in the isolate_or_dissolve_huge_folio()
> function can be removed. Additionally, a similar issue exists in the
> replace_free_hugepage_folios() function, and it should be addressed as
> well.
> 
> Suggested-by: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: Ge Yang <yangge1116@126.com>

Acked-by: Oscar Salvador <osalvador@suse.de>


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[Muchun Song]

> On May 23, 2025, at 16:05, yangge1116@126.com wrote:
> 
> From: Ge Yang <yangge1116@126.com>
> 
> In the isolate_or_dissolve_huge_folio() function, the folio_test_hugetlb()
> function is called to determine whether a folio is a hugetlb folio.
> However, in the subsequent alloc_and_dissolve_hugetlb_folio() function,
> the folio_test_hugetlb() function is called again to make the same
> determination about whether the folio is a hugetlb folio. It appears that
> the folio_test_hugetlb() check in the isolate_or_dissolve_huge_folio()
> function can be removed. Additionally, a similar issue exists in the
> replace_free_hugepage_folios() function, and it should be addressed as
> well.
> 
> Suggested-by: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: Ge Yang <yangge1116@126.com <mailto:yangge1116@126.com>>

Reviewed-by: Muchun Song <muchun.song@linux.dev>

Thanks.

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[David Hildenbrand]

On 23.05.25 10:05, yangge1116@126.com wrote:
> From: Ge Yang <yangge1116@126.com>
> 
> In the isolate_or_dissolve_huge_folio() function, the folio_test_hugetlb()
> function is called to determine whether a folio is a hugetlb folio.
> However, in the subsequent alloc_and_dissolve_hugetlb_folio() function,
> the folio_test_hugetlb() function is called again to make the same
> determination about whether the folio is a hugetlb folio. It appears that
> the folio_test_hugetlb() check in the isolate_or_dissolve_huge_folio()
> function can be removed. Additionally, a similar issue exists in the
> replace_free_hugepage_folios() function, and it should be addressed as
> well.
> 
> Suggested-by: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: Ge Yang <yangge1116@126.com>
> ---
>   mm/hugetlb.c | 51 +++++++++++++--------------------------------------
>   1 file changed, 13 insertions(+), 38 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 6c2e007..6e46f2f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2787,20 +2787,24 @@ void restore_reserve_on_error(struct hstate *h, struct vm_area_struct *vma,
>   /*
>    * alloc_and_dissolve_hugetlb_folio - Allocate a new folio and dissolve
>    * the old one
> - * @h: struct hstate old page belongs to
>    * @old_folio: Old folio to dissolve
>    * @list: List to isolate the page in case we need to
>    * Returns 0 on success, otherwise negated error.
>    */
> -static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
> -			struct folio *old_folio, struct list_head *list)
> +static int alloc_and_dissolve_hugetlb_folio(struct folio *old_folio,
> +			struct list_head *list)
>   {
> -	gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
> +	gfp_t gfp_mask;
> +	struct hstate *h;
>   	int nid = folio_nid(old_folio);
>   	struct folio *new_folio = NULL;
>   	int ret = 0;
>   
>   retry:
> +	/*
> +	 * The old_folio might have been dissolved from under our feet, so make sure
> +	 * to carefully check the state under the lock.
> +	 */
>   	spin_lock_irq(&hugetlb_lock);
>   	if (!folio_test_hugetlb(old_folio)) {
>   		/*
> @@ -2829,8 +2833,10 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
>   		cond_resched();
>   		goto retry;
>   	} else {
> +		h = folio_hstate(old_folio);
>   		if (!new_folio) {
>   			spin_unlock_irq(&hugetlb_lock);
> +			gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>   			new_folio = alloc_buddy_hugetlb_folio(h, gfp_mask, nid,
>   							      NULL, NULL);
>   			if (!new_folio)
> @@ -2874,35 +2880,20 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
>   
>   int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
>   {
> -	struct hstate *h;
>   	int ret = -EBUSY;
>   
>   	/*
> -	 * The page might have been dissolved from under our feet, so make sure
> -	 * to carefully check the state under the lock.
> -	 * Return success when racing as if we dissolved the page ourselves.
> -	 */
> -	spin_lock_irq(&hugetlb_lock);
> -	if (folio_test_hugetlb(folio)) {
> -		h = folio_hstate(folio);
> -	} else {
> -		spin_unlock_irq(&hugetlb_lock);
> -		return 0;
> -	}
> -	spin_unlock_irq(&hugetlb_lock);
> -
> -	/*
>   	 * Fence off gigantic pages as there is a cyclic dependency between
>   	 * alloc_contig_range and them. Return -ENOMEM as this has the effect
>   	 * of bailing out right away without further retrying.
>   	 */
> -	if (hstate_is_gigantic(h))
> +	if (folio_order(folio) > MAX_PAGE_ORDER)
>   		return -ENOMEM;
>   
>   	if (folio_ref_count(folio) && folio_isolate_hugetlb(folio, list))
>   		ret = 0;
>   	else if (!folio_ref_count(folio))
> -		ret = alloc_and_dissolve_hugetlb_folio(h, folio, list);
> +		ret = alloc_and_dissolve_hugetlb_folio(folio, list);
>   
>   	return ret;
>   }
> @@ -2916,7 +2907,6 @@ int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
>    */
>   int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
>   {
> -	struct hstate *h;
>   	struct folio *folio;
>   	int ret = 0;
>   
> @@ -2925,23 +2915,8 @@ int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
>   	while (start_pfn < end_pfn) {
>   		folio = pfn_folio(start_pfn);
>   
> -		/*
> -		 * The folio might have been dissolved from under our feet, so make sure
> -		 * to carefully check the state under the lock.
> -		 */
> -		spin_lock_irq(&hugetlb_lock);
> -		if (folio_test_hugetlb(folio)) {
> -			h = folio_hstate(folio);
> -		} else {
> -			spin_unlock_irq(&hugetlb_lock);
> -			start_pfn++;
> -			continue;
> -		}
> -		spin_unlock_irq(&hugetlb_lock);

^ oh my, that is bad code.

Taking the hugetlb_lock for each and ever page in the range.

Let me find that code and nack it.

> -
>   		if (!folio_ref_count(folio)) {
> -			ret = alloc_and_dissolve_hugetlb_folio(h, folio,
> -							       &isolate_list);
> +			ret = alloc_and_dissolve_hugetlb_folio(folio, &isolate_list);

And we're still doing that there now?

We *really* should have an early folio_test_hugetlb() check and skip 
whatever has no indication of being related to hugetlb.

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[David Hildenbrand]

On 26.05.25 14:20, David Hildenbrand wrote:
> On 23.05.25 10:05, yangge1116@126.com wrote:
>> From: Ge Yang <yangge1116@126.com>
>>
>> In the isolate_or_dissolve_huge_folio() function, the folio_test_hugetlb()
>> function is called to determine whether a folio is a hugetlb folio.
>> However, in the subsequent alloc_and_dissolve_hugetlb_folio() function,
>> the folio_test_hugetlb() function is called again to make the same
>> determination about whether the folio is a hugetlb folio. It appears that
>> the folio_test_hugetlb() check in the isolate_or_dissolve_huge_folio()
>> function can be removed. Additionally, a similar issue exists in the
>> replace_free_hugepage_folios() function, and it should be addressed as
>> well.
>>
>> Suggested-by: Oscar Salvador <osalvador@suse.de>
>> Signed-off-by: Ge Yang <yangge1116@126.com>
>> ---
>>    mm/hugetlb.c | 51 +++++++++++++--------------------------------------
>>    1 file changed, 13 insertions(+), 38 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 6c2e007..6e46f2f 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -2787,20 +2787,24 @@ void restore_reserve_on_error(struct hstate *h, struct vm_area_struct *vma,
>>    /*
>>     * alloc_and_dissolve_hugetlb_folio - Allocate a new folio and dissolve
>>     * the old one
>> - * @h: struct hstate old page belongs to
>>     * @old_folio: Old folio to dissolve
>>     * @list: List to isolate the page in case we need to
>>     * Returns 0 on success, otherwise negated error.
>>     */
>> -static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
>> -			struct folio *old_folio, struct list_head *list)
>> +static int alloc_and_dissolve_hugetlb_folio(struct folio *old_folio,
>> +			struct list_head *list)
>>    {
>> -	gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>> +	gfp_t gfp_mask;
>> +	struct hstate *h;
>>    	int nid = folio_nid(old_folio);
>>    	struct folio *new_folio = NULL;
>>    	int ret = 0;
>>    
>>    retry:
>> +	/*
>> +	 * The old_folio might have been dissolved from under our feet, so make sure
>> +	 * to carefully check the state under the lock.
>> +	 */
>>    	spin_lock_irq(&hugetlb_lock);
>>    	if (!folio_test_hugetlb(old_folio)) {
>>    		/*
>> @@ -2829,8 +2833,10 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
>>    		cond_resched();
>>    		goto retry;
>>    	} else {
>> +		h = folio_hstate(old_folio);
>>    		if (!new_folio) {
>>    			spin_unlock_irq(&hugetlb_lock);
>> +			gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>>    			new_folio = alloc_buddy_hugetlb_folio(h, gfp_mask, nid,
>>    							      NULL, NULL);
>>    			if (!new_folio)
>> @@ -2874,35 +2880,20 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
>>    
>>    int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
>>    {
>> -	struct hstate *h;
>>    	int ret = -EBUSY;
>>    
>>    	/*
>> -	 * The page might have been dissolved from under our feet, so make sure
>> -	 * to carefully check the state under the lock.
>> -	 * Return success when racing as if we dissolved the page ourselves.
>> -	 */
>> -	spin_lock_irq(&hugetlb_lock);
>> -	if (folio_test_hugetlb(folio)) {
>> -		h = folio_hstate(folio);
>> -	} else {
>> -		spin_unlock_irq(&hugetlb_lock);
>> -		return 0;
>> -	}
>> -	spin_unlock_irq(&hugetlb_lock);
>> -
>> -	/*
>>    	 * Fence off gigantic pages as there is a cyclic dependency between
>>    	 * alloc_contig_range and them. Return -ENOMEM as this has the effect
>>    	 * of bailing out right away without further retrying.
>>    	 */
>> -	if (hstate_is_gigantic(h))
>> +	if (folio_order(folio) > MAX_PAGE_ORDER)
>>    		return -ENOMEM;
>>    
>>    	if (folio_ref_count(folio) && folio_isolate_hugetlb(folio, list))
>>    		ret = 0;
>>    	else if (!folio_ref_count(folio))
>> -		ret = alloc_and_dissolve_hugetlb_folio(h, folio, list);
>> +		ret = alloc_and_dissolve_hugetlb_folio(folio, list);
>>    
>>    	return ret;
>>    }
>> @@ -2916,7 +2907,6 @@ int isolate_or_dissolve_huge_folio(struct folio *folio, struct list_head *list)
>>     */
>>    int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
>>    {
>> -	struct hstate *h;
>>    	struct folio *folio;
>>    	int ret = 0;
>>    
>> @@ -2925,23 +2915,8 @@ int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
>>    	while (start_pfn < end_pfn) {
>>    		folio = pfn_folio(start_pfn);
>>    
>> -		/*
>> -		 * The folio might have been dissolved from under our feet, so make sure
>> -		 * to carefully check the state under the lock.
>> -		 */
>> -		spin_lock_irq(&hugetlb_lock);
>> -		if (folio_test_hugetlb(folio)) {
>> -			h = folio_hstate(folio);
>> -		} else {
>> -			spin_unlock_irq(&hugetlb_lock);
>> -			start_pfn++;
>> -			continue;
>> -		}
>> -		spin_unlock_irq(&hugetlb_lock);
> 
> ^ oh my, that is bad code.
> 
> Taking the hugetlb_lock for each and ever page in the range.
> 
> Let me find that code and nack it.

Already in 6.15, gah.

Please convert that code to never ever take any hugeglb locks unless we 
are clearly dealing with a hugetlb folios.

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[Oscar Salvador]

On Mon, May 26, 2025 at 02:23:58PM +0200, David Hildenbrand wrote:
> Already in 6.15, gah.
> 
> Please convert that code to never ever take any hugeglb locks unless we are
> clearly dealing with a hugetlb folios.

I guess we could just do?

 diff --git a/mm/hugetlb.c b/mm/hugetlb.c
 index 20f08de9e37d..8bd8b950e4cb 100644
 --- a/mm/hugetlb.c
 +++ b/mm/hugetlb.c
 @@ -2911,7 +2914,7 @@ int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
  	while (start_pfn < end_pfn) {
  		folio = pfn_folio(start_pfn);
 
 -		if (!folio_ref_count(folio)) {
 +		if (!folio_ref_count(folio) && folio_test_hugetlb(folio)) {
  			ret = alloc_and_dissolve_hugetlb_folio(folio,
  							       &isolate_list);
  			if (ret)
 
 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[David Hildenbrand]

On 26.05.25 15:05, Oscar Salvador wrote:
> On Mon, May 26, 2025 at 02:23:58PM +0200, David Hildenbrand wrote:
>> Already in 6.15, gah.
>>
>> Please convert that code to never ever take any hugeglb locks unless we are
>> clearly dealing with a hugetlb folios.
> 
> I guess we could just do?

Yes, anything that involves hugetlb only id there is an indication of 
... hugetlb :)

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[Oscar Salvador]

On Mon, May 26, 2025 at 03:06:41PM +0200, David Hildenbrand wrote:
> Yes, anything that involves hugetlb only id there is an indication of ...
> hugetlb :)

Yes, I guess that after weeks of only seeing hugetlb code I became
convinced everything is hugetlb :-D.

I will send a patch later today unless no one beats me before.

Thanks for noticing!


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH] mm/hugetlb: remove redundant folio_test_hugetlb

[Ge Yang]

在 2025/5/26 21:06, David Hildenbrand 写道:
> On 26.05.25 15:05, Oscar Salvador wrote:
>> On Mon, May 26, 2025 at 02:23:58PM +0200, David Hildenbrand wrote:
>>> Already in 6.15, gah.
>>>
>>> Please convert that code to never ever take any hugeglb locks unless 
>>> we are
>>> clearly dealing with a hugetlb folios.
>>
>> I guess we could just do?
> 
> Yes, anything that involves hugetlb only id there is an indication 
> of ... hugetlb :)
> 

Okay, thanks. I'll adjust the patch again.