Hi,
On Mon, Dec 16, 2024 at 04:39:12PM +0100, Michal Hocko wrote:
> On Thu 12-12-24 13:30:12, Johannes Weiner wrote:
> [...]
> > So I'm also inclined to think this needs a reclaim/memcg-side fix. We
> > have a somewhat tumultous history of policy in that space:
> >
> > commit 7775face207922ea62a4e96b9cd45abfdc7b9840
> > Author: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> > Date: Tue Mar 5 15:46:47 2019 -0800
> >
> > memcg: killed threads should not invoke memcg OOM killer
> >
> > allowed dying tasks to simply force all charges and move on. This
> > turned out to be too aggressive; there were instances of exiting,
> > uncontained memcg tasks causing global OOMs. This lead to that:
> >
> > commit a4ebf1b6ca1e011289677239a2a361fde4a88076
> > Author: Vasily Averin <vasily.averin@xxxxxxxxx>
> > Date: Fri Nov 5 13:38:09 2021 -0700
> >
> > memcg: prohibit unconditional exceeding the limit of dying tasks
> >
> > which reverted the bypass rather thoroughly. Now NO dying tasks, *not
> > even OOM victims*, can force charges. I am not sure this is correct,
> > either:
>
> IIRC the reason going this route was a lack of per-memcg oom reserves.
> Global oom victims are getting some slack because the amount of reserves
> be bound. This is not the case for memcgs though.
>
> > If we return -ENOMEM to an OOM victim in a fault, the fault handler
> > will re-trigger OOM, which will find the existing OOM victim and do
> > nothing, then restart the fault.
>
> IIRC the task will handle the pending SIGKILL if the #PF fails. If the
> charge happens from the exit path then we rely on ENOMEM returned from
> gup as a signal to back off. Do we have any caller that keeps retrying
> on ENOMEM?
We managed to extract a stack trace of the livelocked task:
obj_cgroup_may_swap
zswap_store
swap_writepage
shrink_folio_list
shrink_lruvec
shrink_node
do_try_to_free_pages
try_to_free_mem_cgroup_pages
charge_memcg
mem_cgroup_swapin_charge_folio
__read_swap_cache_async
swapin_readahead
do_swap_page
handle_mm_fault
do_user_addr_fault
exc_page_fault
asm_exc_page_fault
__get_user
futex_cleanup
fuxtex_exit_release
do_exit
do_group_exit
get_signal
arch_do_signal_or_restart
exit_to_user_mode_prepare
syscall_exit_to_user_mode
do_syscall
entry_SYSCALL_64
syscall
Both memory.max and memory.zswap.max are hit. I don't see how this
could ever make forward progress - the futex fault will retry until it
succeeds. The only workaround for this state right now is to manually
raise memory.max to let the fault succeed and the exit complete.
> > This is a memory deadlock. The page
> > allocator gives OOM victims access to reserves for that reason.
>
> > Actually, it looks even worse. For some reason we're not triggering
> > OOM from dying tasks:
> >
> > ret = task_is_dying() || out_of_memory(&oc);
> >
> > Even though dying tasks are in no way privileged or allowed to exit
> > expediently. Why shouldn't they trigger the OOM killer like anybody
> > else trying to allocate memory?
>
> Good question! I suspect this early bail out is based on an assumption
> that a dying task will free up the memory soon so oom killer is
> unnecessary.
Correct. It's not about the kill. The important thing is that at least
one exiting task is getting the extra memory headroom usually afforded
to the OOM victim, to guarantee forward progress in the exit path.
> > As it stands, it seems we have dying tasks getting trapped in an
> > endless fault->reclaim cycle; with no access to the OOM killer and no
> > access to reserves. Presumably this is what's going on here?
>
> As mentioned above this seems really surprising and it would indicate
> that something in the exit path would keep retrying when getting ENOMEM
> from gup or GFP_ACCOUNT allocation. GFP_NOFAIL requests are allowed to
> over-consume.
I hope the path is clear from the stack trace above.
> > I think we want something like this:
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 53db98d2c4a1..be6b6e72bde5 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1596,11 +1596,7 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
> > if (mem_cgroup_margin(memcg) >= (1 << order))
> > goto unlock;
> >
> > - /*
> > - * A few threads which were not waiting at mutex_lock_killable() can
> > - * fail to bail out. Therefore, check again after holding oom_lock.
> > - */
> > - ret = task_is_dying() || out_of_memory(&oc);
> > + ret = out_of_memory(&oc);
>
> I am not against this as it would allow to do an async oom_reaper memory
> reclaim in the worst case. This could potentially reintroduce the "No
> victim available" case described by 7775face2079 ("memcg: killed threads
> should not invoke memcg OOM killer") but that seemed to be a very
> specific and artificial usecase IIRC.
+1
> > unlock:
> > mutex_unlock(&oom_lock);
> > @@ -2198,6 +2194,9 @@ int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
> > if (unlikely(current->flags & PF_MEMALLOC))
> > goto force;
> >
> > + if (unlikely(tsk_is_oom_victim(current)))
> > + goto force;
> > +
> > if (unlikely(task_in_memcg_oom(current)))
> > goto nomem;
>
> This is more problematic as it doesn't cap a potential runaway and
> eventual global OOM which is not really great. In the past this could be
> possible through vmalloc which didn't bail out early for killed tasks.
> That risk has been mitigated by dd544141b9eb ("vmalloc: back off when
> the current task is OOM-killed"). I would like to keep some sort of
> protection from those runaways. Whether that is a limited "reserve" for
> oom victims that would be per memcg or do no let them consume above the
> hard limit at all. Fundamentally a limited reserves doesn't solve the
> underlying problem, it just make it less likely so the latter would be
> preferred by me TBH.
Right. There is no way to limit an OOM victim without risking a memory
deadlock.
