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@xxxxxxx wrote:
> > From: Ge Yang <yangge1116@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Ge Yang <yangge1116@xxxxxxx>
> > ---
> >    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