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

[zhongjinji <zhongjinji@honor.com>]

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

 static void queue_oom_reaper(struct task_struct *tsk)
 {
+       bool delay = !frozen(tsk);
+
+       /*
+        * Make sure that the task is woken up from uninterruptible sleep
+        * if it is frozen because OOM killer wouldn't be able to free
+        * any memory and livelock. freezing_slow_path will tell the freezer
+        * that TIF_MEMDIE tasks should be ignored.
+        */
+       __thaw_task(tsk);
+
        /* mm is already queued? */
        if (test_and_set_bit(MMF_OOM_REAP_QUEUED, &tsk->signal->oom_mm->flags))
                return;
@@ -711,7 +721,7 @@ static void queue_oom_reaper(struct task_struct *tsk)
         * If the task is frozen by the cgroup freezer, the delay is unnecessary
         * because it cannot exit until thawed. Skip the delay for frozen victims.
         */
-       if (!frozen(tsk))
+       if (delay)
                tsk->oom_reaper_timer.expires += OOM_REAPER_DELAY;
        add_timer(&tsk->oom_reaper_timer);
 }
@@ -783,13 +793,6 @@ static void mark_oom_victim(struct task_struct *tsk)
        if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
                mmgrab(tsk->signal->oom_mm);

-       /*
-        * Make sure that the task is woken up from uninterruptible sleep
-        * if it is frozen because OOM killer wouldn't be able to free
-        * any memory and livelock. freezing_slow_path will tell the freezer
-        * that TIF_MEMDIE tasks should be ignored.
-        */
-       __thaw_task(tsk);
        atomic_inc(&oom_victims);
        cred = get_task_cred(tsk);
        trace_mark_victim(tsk, cred->uid.val);

> 
> > ---
> >  mm/oom_kill.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 25923cfec9c6..a5e9074896a1 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -700,7 +700,14 @@ static void queue_oom_reaper(struct task_struct *tsk)
> >  
> >  	get_task_struct(tsk);
> >  	timer_setup(&tsk->oom_reaper_timer, wake_oom_reaper, 0);
> > -	tsk->oom_reaper_timer.expires = jiffies + OOM_REAPER_DELAY;
> > +	tsk->oom_reaper_timer.expires = jiffies;
> > +
> > +	/*
> > +	 * If the task is frozen by the cgroup freezer, the delay is unnecessary
> > +	 * because it cannot exit until thawed. Skip the delay for frozen victims.
> > +	 */
> > +	if (!frozen(tsk))
> > +		tsk->oom_reaper_timer.expires += OOM_REAPER_DELAY;
> >  	add_timer(&tsk->oom_reaper_timer);
> >  }
> >  
> > -- 
> > 2.17.1