Re: [RFC] Robust futex causing memcg OOM storm on exit

[Michal Hocko <mhocko@suse.com>]

On Wed 22-07-26 17:19:07, Shakeel Butt wrote:
> At Meta, we are seeing instances where an OOM killed job is stuck in the
> exit path for several hours. In one particular case, the job was stuck
> for more than 8 hours and I had to manually remove the memory.max limits
> to allow the process to exit.
> 
> The job was a single process job and had ~55 GiB memory.max and zswap
> enabled. It had almost 0 anon in memory and ~111 GiB in zswap compressed
> to ~51 GiB zswap pool (i.e. almost all of memory.current was zswap).
> Nothing was left on the LRUs to reclaim.
> 
> On further inspection, I observed ~20k threads of that process stuck
> with the following stack:
> 
> [<0>] mem_cgroup_out_of_memory+0x4e/0xa0
> [<0>] charge_memcg+0x8bf/0x990
> [<0>] mem_cgroup_swapin_charge_folio+0x4e/0x80
> [<0>] __read_swap_cache_async+0x10c/0x260
> [<0>] swapin_readahead+0x116/0x3f0
> [<0>] do_swap_page+0x13c/0x1ce0
> [<0>] handle_mm_fault+0x61d/0x11f0
> [<0>] do_user_addr_fault+0x3e7/0x6d0
> [<0>] exc_page_fault+0x8f/0x110
> [<0>] asm_exc_page_fault+0x22/0x30
> [<0>] __get_user_8+0x14/0x20
> [<0>] futex_cleanup+0x27/0x1c0
> [<0>] futex_exit_release+0x47/0x60
> [<0>] do_exit+0x107/0x940
> [<0>] do_group_exit+0x81/0xa0
> [<0>] get_signal+0x2b1/0x6e0
> [<0>] arch_do_signal_or_restart+0x1a/0x1c0
> [<0>] exit_to_user_mode_loop+0xa8/0x1c0
> [<0>] do_syscall_64+0x152/0x250
> [<0>] entry_SYSCALL_64_after_hwframe+0x4b/0x53
> 
> In addition the dmesg was filled with "Out of memory and no killable
> processes..." messages.
> 
> I have no idea why oom reaper was not able to reap/unmap the process. My
> guess is that since oom reaper tries to acquire mmap_lock in read mode
> limited number of times and then gives up, there might a thread of that
> process which had mmap_lock in write mode at that time.
> 
> My initial suspicion was the futex_cleanup and kernel page fault causing
> infinite fault and charge retries but that was put to rest in previous
> discussions happened on similar problem [1].
> 
> My current theory is that it is just a simple slow serialization behind
> the oom_lock. Unlike page allocator, memcg charge code takes the
> oom_lock without the "try". Though memcg oom code uses
> mutex_lock_killable(),  note that in the call stack get_stack() consumes
> SIGKILL (or sigdelset(SIGKILL)) before calling do_cgroup_exit(). So this
> mutex_lock_killable() is just a mutex_lock() here. Therefore 10s of
> thousands of threads are waiting on oom_lock and one by one they get
> -EFAULT from get_user() in the futex cleanup code and bails out.
> 
> Discussion from [1] lead to the commit a75ffa26122b ("memcg, oom: do not
> bypass oom killer for dying tasks") which routes dying tasks into the OOM
> path precisely so the oom_reaper can reap their mm and free the memory
> asynchronously. But the reaper is best-effort and one-shot: if it cannot
> take mmap_lock for read (e.g. a sibling thread holds it for write) it
> sets MMF_OOM_SKIP and never retries, leaving only the glacial
> oom_lock-serialized synchronous drain.
> 
> Let's short-circuit that path: once reclaim has failed, if current is
> dying, force the charge instead of invoking the OOM killer for it. A
> dying task frees its memory as soon as it finishes exiting, so running
> the (necessarily no-victim) OOM killer for it is pointless - and doing so
> for 10s of thousands of exiting threads is exactly what serializes them
> behind oom_lock. The dying task instead faults its page in, completes
> exit and releases its memory, including the zswap pool, so the memcg
> recovers on its own without the oom_lock serialization and dump_header
> storm.

TBH I am not entirely happy about this approach. It effectivelly reverts
a75ffa26122b ("memcg, oom: do not bypass oom killer for dying tasks").
It just makes it lockless. Assumption that a dying task will do so
quickly and with bounded resources has turned wrong on several
occasions.

On the other hand I do undestand the contention issues and I can imagine
that the existing solution doesn't really work well for huge thread
groups that all end up lining up on the oom_lock just to learn there is
nothing really killable anymore because they are the oom victim...

Would it be just safer to bail out only for oom victim threads. This
would narrow down potential runaways for oom victims which should be
more limited than any killed/exiting task. It would also give the oom
killer/reaper chance to work. WDYT?
--- 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6dc4888a90f3..3e0a6b601767 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2685,6 +2685,15 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	if (gfp_mask & __GFP_RETRY_MAYFAIL)
 		goto nomem;
 
+	/*
+	 * OOM victim still needs to charge memory to exit. OOM reaper should
+	 * help but it might fail on mmap_lock contention. If the victim is a
+	 * large thread group then all exiting threads might compete on oom_lock
+	 * just to learn that there is nothing really killable anymore. Bail
+	 * out early and force the charge to expedite their exit.
+	 */
+	if (tsk_is_oom_victim(current))
+		goto force
 	/* Avoid endless loop for tasks bypassed by the oom killer */
 	if (passed_oom && task_is_dying())
 		goto nomem;
-- 
Michal Hocko
SUSE Labs