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

[David Hildenbrand <david@redhat.com>]

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