[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250901093057.27056-1-zhongjinji@honor.com>
Date: Mon, 1 Sep 2025 17:30:57 +0800
From: zhongjinji <zhongjinji@...or.com>
To: <mhocko@...e.com>
CC: <akpm@...ux-foundation.org>, <feng.han@...or.com>,
<fengbaopeng@...or.com>, <liam.howlett@...cle.com>,
<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>, <liulu.liu@...or.com>,
<lorenzo.stoakes@...cle.com>, <rientjes@...gle.com>,
<shakeel.butt@...ux.dev>, <surenb@...gle.com>, <tglx@...utronix.de>,
<tianxiaobin@...or.com>, <zhongjinji@...or.com>
Subject: Re: [PATCH v6 1/2] mm/oom_kill: Do not delay oom reaper when the victim is frozen
> 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@...or.com>
>
> Acked-by: Michal Hocko <mhocko@...e.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
Powered by blists - more mailing lists