Re: [PATCH mm-unstable v3 3/3] mm/page_alloc: simplify __alloc_pages_slowpath() flow

[Michal Hocko <mhocko@suse.com>]

On Tue 06-01-26 12:52:38, Vlastimil Babka wrote:
> The actions done before entering the main retry loop include waking up
> kswapds and an allocation attempt with the precise alloc_flags.
> Then in the loop we keep waking up kswapds, and we retry the allocation
> with flags potentially further adjusted by being allowed to use reserves
> (due to e.g. becoming an OOM killer victim).
> 
> We can adjust the retry loop to keep only one instance of waking up
> kswapds and allocation attempt. Introduce the can_retry_reserves
> variable for retrying once when we become eligible for reserves. It is
> still useful not to evaluate reserve_flags immediately for the first
> allocation attempt, because it's better to first try succeed in a
> non-preferred zone above the min watermark before allocating immediately
> from the preferred zone below min watermark.
> 
> Additionally move the cpuset update checks introduced by e05741fb10c3
> ("mm/page_alloc.c: avoid infinite retries caused by cpuset race")
> further down the retry loop. It's enough to do the checks only before
> reaching any potentially infinite 'goto retry;' loop.
> 
> There should be no meaningful functional changes. The change of exact
> moments the retry for reserves and cpuset updates are checked should not
> result in different outomes modulo races with concurrent allocator
> activity.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

LGTM
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3b2579c5716f..c02564042618 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4716,6 +4716,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	unsigned int zonelist_iter_cookie;
>  	int reserve_flags;
>  	bool compact_first = false;
> +	bool can_retry_reserves = true;
>  
>  	if (unlikely(nofail)) {
>  		/*
> @@ -4783,6 +4784,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  			goto nopage;
>  	}
>  
> +retry:
> +	/* Ensure kswapd doesn't accidentally go to sleep as long as we loop */
>  	if (alloc_flags & ALLOC_KSWAPD)
>  		wake_all_kswapds(order, gfp_mask, ac);
>  
> @@ -4794,19 +4797,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (page)
>  		goto got_pg;
>  
> -retry:
> -	/*
> -	 * Deal with possible cpuset update races or zonelist updates to avoid
> -	 * infinite retries.
> -	 */
> -	if (check_retry_cpuset(cpuset_mems_cookie, ac) ||
> -	    check_retry_zonelist(zonelist_iter_cookie))
> -		goto restart;
> -
> -	/* Ensure kswapd doesn't accidentally go to sleep as long as we loop */
> -	if (alloc_flags & ALLOC_KSWAPD)
> -		wake_all_kswapds(order, gfp_mask, ac);
> -
>  	reserve_flags = __gfp_pfmemalloc_flags(gfp_mask);
>  	if (reserve_flags)
>  		alloc_flags = gfp_to_alloc_flags_cma(gfp_mask, reserve_flags) |
> @@ -4821,12 +4811,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		ac->nodemask = NULL;
>  		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
>  					ac->highest_zoneidx, ac->nodemask);
> -	}
>  
> -	/* Attempt with potentially adjusted zonelist and alloc_flags */
> -	page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac);
> -	if (page)
> -		goto got_pg;
> +		/*
> +		 * The first time we adjust anything due to being allowed to
> +		 * ignore memory policies or watermarks, retry immediately. This
> +		 * allows us to keep the first allocation attempt optimistic so
> +		 * it can succeed in a zone that is still above watermarks.
> +		 */
> +		if (can_retry_reserves) {
> +			can_retry_reserves = false;
> +			goto retry;
> +		}
> +	}
>  
>  	/* Caller is not willing to reclaim, we can't balance anything */
>  	if (!can_direct_reclaim)
> @@ -4889,6 +4885,15 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  			     !(gfp_mask & __GFP_RETRY_MAYFAIL)))
>  		goto nopage;
>  
> +	/*
> +	 * Deal with possible cpuset update races or zonelist updates to avoid
> +	 * infinite retries. No "goto retry;" can be placed above this check
> +	 * unless it can execute just once.
> +	 */
> +	if (check_retry_cpuset(cpuset_mems_cookie, ac) ||
> +	    check_retry_zonelist(zonelist_iter_cookie))
> +		goto restart;
> +
>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
>  				 did_some_progress > 0, &no_progress_loops))
>  		goto retry;
> 
> -- 
> 2.52.0

-- 
Michal Hocko
SUSE Labs