Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Michal Hocko <mhocko@suse.com>]

On Thu 06-08-26 14:24:28, Audra Mitchell wrote:
> Originally nr_retries was actually nr_oom_retries and we used it to track (and
> limit) the number of times we entered the mem_cgroup_oom path and then attempted
> a retry. The purpose of nr_retries counter changed with the introduction of
> 9b1306192d33 ("mm: memcontrol: retry reclaim for oom-disabled and __GFP_NOFAIL
> charges") so that the oom-disabled and __GFP_NOFAIL charges would also continue
> to retry within the desired nr_retries threshold. Later d977aa939fca
> ("mm, memcg: unify reclaim retry limits with page allocator") changed the
> nr_retries counter from 5 to 16.
> 
> As the function has evolved we now have multiple paths that have a goto retry
> path and we have lost the original purpose of the nr_retries counter, allowing
> us to take a goto retry path an unbounded number of times.
> 
> Fix the unbounded retries by nesting the code in a loop and decrementing the
> nr_retries counter correctly.

Are you trying to fix a theoretical problem spotted by the code review
or is there any actual problem that you are trying to fix?

> 
> Signed-off-by: Audra Mitchell <audra@redhat.com>
> ---
>  mm/memcontrol.c | 153 ++++++++++++++++++++++++------------------------
>  1 file changed, 76 insertions(+), 77 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6dc4888a90f3..781bcced5848 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2607,98 +2607,97 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	unsigned long pflags;
>  	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
>  
> -retry:
> -	if (consume_stock(memcg, nr_pages))
> -		return 0;
> +	for (; nr_retries >= 0; nr_retries--) {
>  
> -	if (!allow_spinning)
> -		/* Avoid the refill and flush of the older stock */
> -		batch = nr_pages;
> +		if (consume_stock(memcg, nr_pages))
> +			return 0;
>  
> -	reclaim_options = MEMCG_RECLAIM_MAY_SWAP;
> -	if (!do_memsw_account() ||
> -	    page_counter_try_charge(&memcg->memsw, batch, &counter)) {
> -		if (page_counter_try_charge(&memcg->memory, batch, &counter))
> -			goto done_restock;
> -		if (do_memsw_account())
> -			page_counter_uncharge(&memcg->memsw, batch);
> -		mem_over_limit = mem_cgroup_from_counter(counter, memory);
> -	} else {
> -		mem_over_limit = mem_cgroup_from_counter(counter, memsw);
> -		reclaim_options &= ~MEMCG_RECLAIM_MAY_SWAP;
> -	}
> +		if (!allow_spinning)
> +			/* Avoid the refill and flush of the older stock */
> +			batch = nr_pages;
>  
> -	if (batch > nr_pages) {
> -		batch = nr_pages;
> -		goto retry;
> -	}
> +		reclaim_options = MEMCG_RECLAIM_MAY_SWAP;
> +		if (!do_memsw_account() ||
> +		    page_counter_try_charge(&memcg->memsw, batch, &counter)) {
> +			if (page_counter_try_charge(&memcg->memory, batch, &counter))
> +				goto done_restock;
> +			if (do_memsw_account())
> +				page_counter_uncharge(&memcg->memsw, batch);
> +			mem_over_limit = mem_cgroup_from_counter(counter, memory);
> +		} else {
> +			mem_over_limit = mem_cgroup_from_counter(counter, memsw);
> +			reclaim_options &= ~MEMCG_RECLAIM_MAY_SWAP;
> +		}
>  
> -	/*
> -	 * Prevent unbounded recursion when reclaim operations need to
> -	 * allocate memory. This might exceed the limits temporarily,
> -	 * but we prefer facilitating memory reclaim and getting back
> -	 * under the limit over triggering OOM kills in these cases.
> -	 */
> -	if (unlikely(current->flags & PF_MEMALLOC))
> -		goto force;
> +		if (batch > nr_pages) {
> +			batch = nr_pages;
> +			continue;
> +		}
>  
> -	if (unlikely(task_in_memcg_oom(current)))
> -		goto nomem;
> +		/*
> +		 * Prevent unbounded recursion when reclaim operations need to
> +		 * allocate memory. This might exceed the limits temporarily,
> +		 * but we prefer facilitating memory reclaim and getting back
> +		 * under the limit over triggering OOM kills in these cases.
> +		 */
> +		if (unlikely(current->flags & PF_MEMALLOC))
> +			goto force;
>  
> -	if (!gfpflags_allow_blocking(gfp_mask))
> -		goto nomem;
> +		if (unlikely(task_in_memcg_oom(current)))
> +			goto nomem;
>  
> -	__memcg_memory_event(mem_over_limit, MEMCG_MAX, allow_spinning);
> -	raised_max_event = true;
> +		if (!gfpflags_allow_blocking(gfp_mask))
> +			goto nomem;
>  
> -	psi_memstall_enter(&pflags);
> -	nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
> -						    gfp_mask, reclaim_options, NULL);
> -	psi_memstall_leave(&pflags);
> +		__memcg_memory_event(mem_over_limit, MEMCG_MAX, allow_spinning);
> +		raised_max_event = true;
>  
> -	if (mem_cgroup_margin(mem_over_limit) >= nr_pages)
> -		goto retry;
> +		psi_memstall_enter(&pflags);
> +		nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
> +						    gfp_mask, reclaim_options, NULL);
> +		psi_memstall_leave(&pflags);
>  
> -	if (!drained) {
> -		drain_all_stock(mem_over_limit);
> -		drained = true;
> -		goto retry;
> -	}
> +		if (mem_cgroup_margin(mem_over_limit) >= nr_pages)
> +			continue;
>  
> -	if (gfp_mask & __GFP_NORETRY)
> -		goto nomem;
> -	/*
> -	 * Even though the limit is exceeded at this point, reclaim
> -	 * may have been able to free some pages.  Retry the charge
> -	 * before killing the task.
> -	 *
> -	 * Only for regular pages, though: huge pages are rather
> -	 * unlikely to succeed so close to the limit, and we fall back
> -	 * to regular pages anyway in case of failure.
> -	 */
> -	if (nr_reclaimed && nr_pages <= (1 << PAGE_ALLOC_COSTLY_ORDER))
> -		goto retry;
> +		if (!drained) {
> +			drain_all_stock(mem_over_limit);
> +			drained = true;
> +			continue;
> +		}
>  
> -	if (nr_retries--)
> -		goto retry;
> +		if (gfp_mask & __GFP_NORETRY)
> +			goto nomem;
> +		/*
> +		 * Even though the limit is exceeded at this point, reclaim
> +		 * may have been able to free some pages.  Retry the charge
> +		 * before killing the task.
> +		 *
> +		 * Only for regular pages, though: huge pages are rather
> +		 * unlikely to succeed so close to the limit, and we fall back
> +		 * to regular pages anyway in case of failure.
> +		 */
> +		if (nr_reclaimed && nr_pages <= (1 << PAGE_ALLOC_COSTLY_ORDER))
> +			continue;
>  
> -	if (gfp_mask & __GFP_RETRY_MAYFAIL)
> -		goto nomem;
> +		if (gfp_mask & __GFP_RETRY_MAYFAIL)
> +			goto nomem;
>  
> -	/* Avoid endless loop for tasks bypassed by the oom killer */
> -	if (passed_oom && task_is_dying())
> -		goto nomem;
> +		/* Avoid endless loop for tasks bypassed by the oom killer */
> +		if (passed_oom && task_is_dying())
> +			goto nomem;
>  
> -	/*
> -	 * keep retrying as long as the memcg oom killer is able to make
> -	 * a forward progress or bypass the charge if the oom killer
> -	 * couldn't make any progress.
> -	 */
> -	if (mem_cgroup_oom(mem_over_limit, gfp_mask,
> -			   get_order(nr_pages * PAGE_SIZE))) {
> -		passed_oom = true;
> -		nr_retries = MAX_RECLAIM_RETRIES;
> -		goto retry;
> +		/*
> +		 * keep retrying as long as the memcg oom killer is able to make
> +		 * a forward progress or bypass the charge if the oom killer
> +		 * couldn't make any progress.
> +		 */
> +		if (mem_cgroup_oom(mem_over_limit, gfp_mask,
> +				   get_order(nr_pages * PAGE_SIZE))) {
> +			passed_oom = true;
> +			nr_retries = MAX_RECLAIM_RETRIES;
> +			continue;
> +		}
>  	}
>  nomem:
>  	/*
> -- 
> 2.52.0
> 

-- 
Michal Hocko
SUSE Labs