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

[David Hildenbrand <david@redhat.com>]

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