Re: [PATCH 3/7] mm; hugetlb: simpify alloc_buddy_hugetlb_folio()

["Vishal Moola (Oracle)" <vishal.moola@gmail.com>]

On Sat, Aug 02, 2025 at 03:31:03PM +0800, Kefeng Wang wrote:
> Check folio once instead of three times in alloc_buddy_hugetlb_folio().
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/hugetlb.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index afec5a6a8aca..436403fb0bed 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1949,29 +1949,26 @@ static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h,
>  		nid = numa_mem_id();
>  
>  	folio = (struct folio *)__alloc_frozen_pages(gfp_mask, order, nid, nmask);
> -
> -	/*
> -	 * If we did not specify __GFP_RETRY_MAYFAIL, but still got a
> -	 * folio this indicates an overall state change.  Clear bit so
> -	 * that we resume normal 'try hard' allocations.
> -	 */
> -	if (node_alloc_noretry && folio && !alloc_try_hard)
> -		node_clear(nid, *node_alloc_noretry);
> -
> -	/*
> -	 * If we tried hard to get a folio but failed, set bit so that
> -	 * subsequent attempts will not try as hard until there is an
> -	 * overall state change.
> -	 */
> -	if (node_alloc_noretry && !folio && alloc_try_hard)
> -		node_set(nid, *node_alloc_noretry);
> -
> -	if (!folio) {

I really don't like the way you've done this below. I feel like it makes
the code harder to understand since it feels like we've effectively
divided this function into 2 different ones (depending on if we
successfully got a folio).

I don't have a better idea, but I also don't think eliminating those 2
extra checks is worth increasing the complexity like this.

> +	if (folio) {
> +		/*
> +		 * If we did not specify __GFP_RETRY_MAYFAIL, but still got a
> +		 * folio this indicates an overall state change.  Clear bit so
> +		 * that we resume normal 'try hard' allocations.
> +		 */
> +		if (node_alloc_noretry && !alloc_try_hard)
> +			node_clear(nid, *node_alloc_noretry);
> +		__count_vm_event(HTLB_BUDDY_PGALLOC);
> +	} else {
> +		/*
> +		 * If we tried hard to get a folio but failed, set bit so that
> +		 * subsequent attempts will not try as hard until there is an
> +		 * overall state change.
> +		 */
> +		if (node_alloc_noretry && alloc_try_hard)
> +			node_set(nid, *node_alloc_noretry);
>  		__count_vm_event(HTLB_BUDDY_PGALLOC_FAIL);
> -		return NULL;
>  	}
>  
> -	__count_vm_event(HTLB_BUDDY_PGALLOC);
>  	return folio;
>  }
>  
> -- 
> 2.27.0
> 
>