Re: [PATCH v6 1/2] mm/oom_kill: Do not delay oom reaper when the victim is frozen

[Michal Hocko <mhocko@suse.com>]

On Mon 01-09-25 17:30:57, zhongjinji wrote:
> > On Fri 29-08-25 14:55:49, zhongjinji wrote:
> > > The oom reaper is a mechanism to guarantee a forward process during OOM
> > > situation when the oom victim cannot terminate on its own (e.g. being
> > > blocked in uninterruptible state or frozen by cgroup freezer). In order
> > > to give the victim some time to terminate properly the oom reaper is
> > > delayed in its invocation. This is particularly beneficial when the oom
> > > victim is holding robust futex resources as the anonymous memory tear
> > > down can break those. [1]
> > > 
> > > On the other hand deliberately frozen tasks by the freezer cgroup will
> > > not wake up until they are thawed in the userspace and delay is
> > > effectively pointless. Therefore opt out from the delay for cgroup
> > > frozen oom victims.
> > > 
> > > Reference:
> > > [1] https://lore.kernel.org/all/20220414144042.677008-1-npache@redhat.com/T/#u
> > > 
> > > Signed-off-by: zhongjinji <zhongjinji@honor.com>
> > 
> > Acked-by: Michal Hocko <mhocko@suse.com>
> > Thanks
> 
> Sorry, I found that it doesn't work now (because I previously tested it by
> simulating OOM, which made testing easier but also caused the mistake. I will
> re-run the new test). Calling __thaw_task in mark_oom_victim will change the
> victim's state to running. However, other threads are still in the frozen state,
> so the process still can't exit. We should update it again by moving __thaw_task
> to after frozen (this way, executing __thaw_task and frozen in the same function
> looks more reasonable). Since mark_oom_victim and queue_oom_reaper always appear
> in pairs, this won't introduce any risky changes.

Hmm, I must have completely forgot that we are actually thawing the
frozen task! That means that the actual argument for not delaying the
oom reaper doesn't hold.
Now I do see why the existing implementation doesn't really work as you
would expect though. Is there any reason why we are not thawing the
whole process group? I guess I just didn't realize that __thaw_task is
per thread rather than per process back then when I have introduced it.
Because thread specific behavior makes very little sense to me TBH.
So rather than plaing with __thaw_task placement which doesn't really
make much sense wrt to delaying the reaper we should look into that
part.

Sorry, I should have realized earlier when proposing that.

-- 
Michal Hocko
SUSE Labs