mm/memcontrol.c | 153 ++++++++++++++++++++++++------------------------ 1 file changed, 76 insertions(+), 77 deletions(-)
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.0On 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
© 2016 - 2026 Red Hat, Inc.