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 --- 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