[PATCH] Fix unbounded loop within try_charge_memcg

[PATCH] Fix unbounded loop within try_charge_memcg

[Audra Mitchell]

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.

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

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Michal Hocko]

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

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Audra Mitchell]

On Fri, Aug 07, 2026 at 10:06:53AM +0200, Michal Hocko wrote:
> On Thu 06-08-26 11:10:03, 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?

We have had some customer complaints that performance has slowed to a crawl when
the cgroup memory limit has come close to the maximum limit. In those cases, we
have noticed that each process is spending a large amount of time in the direct
reclaim path acquiring just enough memory for their specific allocation, thus
by-passing the oom condition yet degrading the system's overall performance.
In the global case, direct reclaim is bounded by DEF_PRIORITY, however, a cgroup
will go through the try_charge_memcg path which will call
try_to_free_mem_cgroup_pages->do_try_to_free_pages each time it does a retry (16
times). If we bound the loop in try_charge_memcg, the worst case is 16*12 passes
attempting to reclaim. This patch is meant to address the unbound case, limiting
the loops to 16 attempts at following the direct reclaim path. An argument could
be made to reduce nr_retries as well, but given that the nr_retries has been set
to 16 for sometime, it seemed unlikely such a change would be considered.

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Shakeel Butt]

On Fri, Aug 07, 2026 at 11:40:06AM -0400, Audra Mitchell wrote:
> On Fri, Aug 07, 2026 at 10:06:53AM +0200, Michal Hocko wrote:
> > On Thu 06-08-26 11:10:03, 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?
> 
> We have had some customer complaints that performance has slowed to a crawl when
> the cgroup memory limit has come close to the maximum limit. In those cases, we
> have noticed that each process is spending a large amount of time in the direct
> reclaim path acquiring just enough memory for their specific allocation, thus
> by-passing the oom condition yet degrading the system's overall performance.
> In the global case, direct reclaim is bounded by DEF_PRIORITY, however, a cgroup
> will go through the try_charge_memcg path which will call
> try_to_free_mem_cgroup_pages->do_try_to_free_pages each time it does a retry (16
> times). If we bound the loop in try_charge_memcg, the worst case is 16*12 passes
> attempting to reclaim. This patch is meant to address the unbound case, limiting
> the loops to 16 attempts at following the direct reclaim path. An argument could
> be made to reduce nr_retries as well, but given that the nr_retries has been set
> to 16 for sometime, it seemed unlikely such a change would be considered.
> 

Generally we keep the kernel oom-killer very conservative and let the userspace
system-oomd react and trigger the kill (unless there is an obvious bug and/or does
not require to add one more heuristic in the reclaim+oom path).

Anyways was systemd-oomd and psi enabled on the user system?

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Michal Hocko]

On Fri 07-08-26 11:40:06, Audra Mitchell wrote:
> On Fri, Aug 07, 2026 at 10:06:53AM +0200, Michal Hocko wrote:
> > On Thu 06-08-26 11:10:03, 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?
> 
> We have had some customer complaints that performance has slowed to a crawl when
> the cgroup memory limit has come close to the maximum limit. In those cases, we
> have noticed that each process is spending a large amount of time in the direct
> reclaim path acquiring just enough memory for their specific allocation, thus
> by-passing the oom condition yet degrading the system's overall performance.

Yes, this is entirely possible scenario.

> In the global case, direct reclaim is bounded by DEF_PRIORITY,

Well, both global and memcg reclaim share the reclaim logic. Both of
them try to exercise all reclaim priorities (i.e. check whole eligible
LRU lists) and they fall back to OOM killer only if there is no other
option left. For the global case should_reclaim_retry is the gate keeper
around direct reclaim retries while for the memcg we have more or less
fixed number of retries.

> however, a cgroup
> will go through the try_charge_memcg path which will call
> try_to_free_mem_cgroup_pages->do_try_to_free_pages each time it does a retry (16
> times).

> If we bound the loop in try_charge_memcg, the worst case is 16*12 passes
> attempting to reclaim. This patch is meant to address the unbound case, limiting
> the loops to 16 attempts at following the direct reclaim path. An argument could
> be made to reduce nr_retries as well, but given that the nr_retries has been set
> to 16 for sometime, it seemed unlikely such a change would be considered.

As Shakeel said in other reply, this is a deliberate implementation
decision. The OOM killer is the very last resort and we are giving
chance to userspace to handle close to OOM situation much more
gracefully and also workload aware. Keep in mind that what might be seen
as a slow progress for one workload might be acceptable for others where
OOM killer could mean a lot of work being lost.

From what you are describing above those users might be hitting reclaim trashing.
I.e. last small portion of a reclaimable memory is bounced back and
forth for the workload to make tiny but steady forward progress. While
OOM killer might help to stop the suffering and restart the workload
sooner I would generally recommend revisiting limits set for the
particular workload. Especially if restarting it might lead to the same
state sooner or later. Watching PSI metric would be a good start to see
how the workload behaves wrt memory stalling. User space oom handlers
might be a proper measure as well but that will always be safeguard
rather than a solution.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Shakeel Butt]

On Fri, Aug 07, 2026 at 08:47:10PM +0200, Michal Hocko wrote:
> On Fri 07-08-26 11:40:06, Audra Mitchell wrote:
> > On Fri, Aug 07, 2026 at 10:06:53AM +0200, Michal Hocko wrote:
> > > On Thu 06-08-26 11:10:03, 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?
> > 
> > We have had some customer complaints that performance has slowed to a crawl when
> > the cgroup memory limit has come close to the maximum limit. In those cases, we
> > have noticed that each process is spending a large amount of time in the direct
> > reclaim path acquiring just enough memory for their specific allocation, thus
> > by-passing the oom condition yet degrading the system's overall performance.
> 
> Yes, this is entirely possible scenario.
> 
> > In the global case, direct reclaim is bounded by DEF_PRIORITY,
> 
> Well, both global and memcg reclaim share the reclaim logic. Both of
> them try to exercise all reclaim priorities (i.e. check whole eligible
> LRU lists) and they fall back to OOM killer only if there is no other
> option left. For the global case should_reclaim_retry is the gate keeper
> around direct reclaim retries while for the memcg we have more or less
> fixed number of retries.
> 
> > however, a cgroup
> > will go through the try_charge_memcg path which will call
> > try_to_free_mem_cgroup_pages->do_try_to_free_pages each time it does a retry (16
> > times).
> 
> > If we bound the loop in try_charge_memcg, the worst case is 16*12 passes
> > attempting to reclaim. This patch is meant to address the unbound case, limiting
> > the loops to 16 attempts at following the direct reclaim path. An argument could
> > be made to reduce nr_retries as well, but given that the nr_retries has been set
> > to 16 for sometime, it seemed unlikely such a change would be considered.
> 
> As Shakeel said in other reply, this is a deliberate implementation
> decision. The OOM killer is the very last resort and we are giving
> chance to userspace to handle close to OOM situation much more
> gracefully and also workload aware. Keep in mind that what might be seen
> as a slow progress for one workload might be acceptable for others where
> OOM killer could mean a lot of work being lost.
> 
> From what you are describing above those users might be hitting reclaim trashing.
> I.e. last small portion of a reclaimable memory is bounced back and
> forth for the workload to make tiny but steady forward progress. While
> OOM killer might help to stop the suffering and restart the workload
> sooner I would generally recommend revisiting limits set for the
> particular workload.

+1 to this. Beside memory.pressure, we also have refault and reclaim metrics
in memory.stat which can further help in debugging if the workload is thrashing
due to workingset larger than the limits.

> Especially if restarting it might lead to the same
> state sooner or later. Watching PSI metric would be a good start to see
> how the workload behaves wrt memory stalling. User space oom handlers
> might be a proper measure as well but that will always be safeguard
> rather than a solution.
> -- 
> Michal Hocko
> SUSE Labs

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Audra Mitchell]

On Fri, Aug 07, 2026 at 08:47:10PM +0200, Michal Hocko wrote:
> On Fri 07-08-26 11:40:06, Audra Mitchell wrote:
> > On Fri, Aug 07, 2026 at 10:06:53AM +0200, Michal Hocko wrote:
> > > On Thu 06-08-26 11:10:03, Audra Mitchell wrote:
> 
> Well, both global and memcg reclaim share the reclaim logic. Both of
> them try to exercise all reclaim priorities (i.e. check whole eligible
> LRU lists) and they fall back to OOM killer only if there is no other
> option left. For the global case should_reclaim_retry is the gate keeper
> around direct reclaim retries while for the memcg we have more or less
> fixed number of retries.

> From what you are describing above those users might be hitting reclaim trashing.
> I.e. last small portion of a reclaimable memory is bounced back and
> forth for the workload to make tiny but steady forward progress. While
> OOM killer might help to stop the suffering and restart the workload
> sooner I would generally recommend revisiting limits set for the
> particular workload. Especially if restarting it might lead to the same
> state sooner or later. Watching PSI metric would be a good start to see
> how the workload behaves wrt memory stalling. User space oom handlers
> might be a proper measure as well but that will always be safeguard
> rather than a solution.

Both of these recommendations were made to the end customer, with a
significant emphasis on reviewing the workload and the limits in place.

However, while reviewing the code, I was truly surprised to find retry
pathways in try_charge_memcg that do not decrement the counter, such as this:

        nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
                                                    gfp_mask, reclaim_options, NULL);
        psi_memstall_leave(&pflags);

        if (mem_cgroup_margin(mem_over_limit) >= nr_pages)
                goto retry;

If the intent is to have a counter of nr_retries, I am genuinely surprised
we are comfortable not adhering to said counter. The patch is not meant to
change any heuristic, but only enforce a counter that is already in place.

I have no real dog in the fight of pushing this change to upstream, so if the
patch is not desired I will let this lie. Thank you for taking a look and
the dialouge. Happy Weekend!

Re: [PATCH] Fix unbounded loop within try_charge_memcg

[Michal Hocko]

On Fri 07-08-26 15:13:22, Audra Mitchell wrote:
> On Fri, Aug 07, 2026 at 08:47:10PM +0200, Michal Hocko wrote:
> > On Fri 07-08-26 11:40:06, Audra Mitchell wrote:
> > > On Fri, Aug 07, 2026 at 10:06:53AM +0200, Michal Hocko wrote:
> > > > On Thu 06-08-26 11:10:03, Audra Mitchell wrote:
> > 
> > Well, both global and memcg reclaim share the reclaim logic. Both of
> > them try to exercise all reclaim priorities (i.e. check whole eligible
> > LRU lists) and they fall back to OOM killer only if there is no other
> > option left. For the global case should_reclaim_retry is the gate keeper
> > around direct reclaim retries while for the memcg we have more or less
> > fixed number of retries.
> 
> > From what you are describing above those users might be hitting reclaim trashing.
> > I.e. last small portion of a reclaimable memory is bounced back and
> > forth for the workload to make tiny but steady forward progress. While
> > OOM killer might help to stop the suffering and restart the workload
> > sooner I would generally recommend revisiting limits set for the
> > particular workload. Especially if restarting it might lead to the same
> > state sooner or later. Watching PSI metric would be a good start to see
> > how the workload behaves wrt memory stalling. User space oom handlers
> > might be a proper measure as well but that will always be safeguard
> > rather than a solution.
> 
> Both of these recommendations were made to the end customer, with a
> significant emphasis on reviewing the workload and the limits in place.
> 
> However, while reviewing the code, I was truly surprised to find retry
> pathways in try_charge_memcg that do not decrement the counter, such as this:
> 
>         nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages,
>                                                     gfp_mask, reclaim_options, NULL);
>         psi_memstall_leave(&pflags);
> 
>         if (mem_cgroup_margin(mem_over_limit) >= nr_pages)
>                 goto retry;
> 
> If the intent is to have a counter of nr_retries, I am genuinely surprised
> we are comfortable not adhering to said counter.

The said counter is conuting retries without any real progress.
Similarly to the global reclaim. If we get some margin here we are
making progress so we are not really OOM yet.

> The patch is not meant to
> change any heuristic, but only enforce a counter that is already in place.

It is very much chaning the heuristic ;) With your proposed change we
would be hitting OOM much more eaiser and often. Not something a lot of
user would appreciate.
-- 
Michal Hocko
SUSE Labs