Re: [PATCH 1/1] mm: vmscan: Reduce throttling due to a failure to make progress

[Mike Galbraith]

On Thu, 2021-11-25 at 15:18 +0000, Mel Gorman wrote:
> Mike Galbraith, Alexey Avramov and Darrick Wong all reported similar
> problems due to reclaim throttling for excessive lengths of time.
> In Alexey's case, a memory hog that should go OOM quickly stalls for
> several minutes before stalling. In Mike and Darrick's cases, a small
> memcg environment stalled excessively even though the system had enough
> memory overall.
>
> Commit 69392a403f49 ("mm/vmscan: throttle reclaim when no progress is being
> made") introduced the problem although commit a19594ca4a8b ("mm/vmscan:
> increase the timeout if page reclaim is not making progress") made it
> worse. Systems at or near an OOM state that cannot be recovered must
> reach OOM quickly and memcg should kill tasks if a memcg is near OOM.
>
> To address this, only stall for the first zone in the zonelist, reduce
> the timeout to 1 tick for VMSCAN_THROTTLE_NOPROGRESS and only stall if
> the scan control nr_reclaimed is 0 and kswapd is still active.  If kswapd
> has stopped reclaiming due to excessive failures, do not stall at all so
> that OOM triggers relatively quickly.

This patch helped a ton with LTP testcases, but, the behavior of
test_1() in memcg_regression_test.sh still looks pretty darn bad...

homer:..debug/tracing # tail -1 /trace
    memcg_test_1-4683    [002] .....   282.319617: out_of_memory+0x194/0x440: !!oc->chosen:1
homer:..debug/tracing # grep memcg_test_1-4683 /trace|grep sleepy|wc -l
190 !!!

That one memcg_test_1 instance, of 400 spawned in a memcg with its
limit_in_bytes set to zero, slept 190 times on the way to oom-kill,
leading a regression test that used to complete in a fraction of a
second still taking over 8 minutes to even with the huge improvement
$subject made.

Poking it with the sharp stick below took it down to 20 encounters with
reclaim_throttle(), and a tad under a minute for test_1() to complete.

Reasoning: given try_charge_memcg() will loop as long as there is ANY
progress, and each call to try_to_free_mem_cgroup_pages() therein now
entails multiple encounters with reclaim_throttle(), allowing plenty of
time for some progress enabling events to have occurred and benefit
reaped by the time we return, looping again and again when having been
throttled numerous times did NOT help at all seems now to constitute
pointless thumb twiddling.  Or?

---
 mm/memcontrol.c |    5 -----
 1 file changed, 5 deletions(-)

--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2584,7 +2584,6 @@ static int try_charge_memcg(struct mem_c
 			unsigned int nr_pages)
 {
 	unsigned int batch = max(MEMCG_CHARGE_BATCH, nr_pages);
-	int nr_retries = MAX_RECLAIM_RETRIES;
 	struct mem_cgroup *mem_over_limit;
 	struct page_counter *counter;
 	enum oom_status oom_status;
@@ -2675,9 +2674,6 @@ static int try_charge_memcg(struct mem_c
 	if (mem_cgroup_wait_acct_move(mem_over_limit))
 		goto retry;

-	if (nr_retries--)
-		goto retry;
-
 	if (gfp_mask & __GFP_RETRY_MAYFAIL)
 		goto nomem;

@@ -2694,7 +2690,6 @@ static int try_charge_memcg(struct mem_c
 		       get_order(nr_pages * PAGE_SIZE));
 	if (oom_status == OOM_SUCCESS) {
 		passed_oom = true;
-		nr_retries = MAX_RECLAIM_RETRIES;
 		goto retry;
 	}
 nomem: