Re: [PATCH v8 1/3] mm/oom_kill: Introduce thaw_oom_process() for thawing OOM victims

[zhongjinji <zhongjinji@honor.com>]

> On Tue 09-09-25 17:06:57, zhongjinji wrote:
> > OOM killer is a mechanism that selects and kills processes when the system
> > runs out of memory to reclaim resources and keep the system stable.
> > However, the oom victim cannot terminate on its own when it is frozen,
> > because __thaw_task() only thaws one thread of the victim, while
> > the other threads remain in the frozen state.
> > 
> > Since __thaw_task did not fully thaw the OOM victim for self-termination,
> > introduce thaw_oom_process() to properly thaw OOM victims.
> 
> You will need s@thaw_oom_process@thaw_processes@

The reason for using thaw_oom_process is that the TIF_MEMDIE flag of the
thawed thread will be set, which means this function can only be used to
thaw processes terminated by the OOM killer.

thaw_processes has already been defined in kernel/power/process.c.
Would it be better to use thaw_process instead?

I am concerned that others might misunderstand the thaw_process function.
thaw_process sets all threads to the TIF_MEMDIE state, so it can only be
used to thaw processes killed by the OOM killer.

If the TIF_MEMDIE flag of a thread is not set, the thread cannot be thawed
regardless of the cgroup state. Should we add a function to set the TIF_MEMDIE
state for all threads, like the implementation below?

-/*
- * thaw_oom_process - thaw the OOM victim process
- * @p: process to be thawed
- *
- * Sets TIF_MEMDIE for all threads in the process group and thaws them.
- * Threads with TIF_MEMDIE are ignored by the freezer.
- */
-void thaw_oom_process(struct task_struct *p)
+void thaw_process(struct task_struct *p)
 {
        struct task_struct *t;

        rcu_read_lock();
        for_each_thread(p, t) {
-               set_tsk_thread_flag(t, TIF_MEMDIE);
                __thaw_task(t);
        }
        rcu_read_unlock();
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 52d285da5ba4..67b65b249757 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -753,6 +753,17 @@ static inline void queue_oom_reaper(struct task_struct *tsk)
 }
 #endif /* CONFIG_MMU */

+void mark_oom_victim_die(struct task_struct *p)
+{
+       struct task_struct *t;
+
+       rcu_read_lock();
+       for_each_thread(p, t) {
+               set_tsk_thread_flag(t, TIF_MEMDIE);
+       }
+       rcu_read_unlock();
+}
+
 /**
  * mark_oom_victim - mark the given task as OOM victim
  * @tsk: task to mark
@@ -782,7 +793,8 @@ static void mark_oom_victim(struct task_struct *tsk)
         * if it is frozen because OOM killer wouldn't be able to free
         * any memory and livelock.
         */
-       thaw_oom_process(tsk);
+       mark_oom_victim_die(tsk);
+       thaw_process(tsk);

> I would also add the caller in this patch.
>
> > Signed-off-by: zhongjinji <zhongjinji@honor.com>
> 
> Other than that looks good to me. With the above fixed feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>