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

[Michal Hocko <mhocko@suse.com>]

On Tue 09-09-25 21:51:52, zhongjinji wrote:
> > On Tue 09-09-25 19:41:31, zhongjinji wrote:
> > > > 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.
> > 
> > Just do not set the flag inside the function. I would even say do not
> > set TIF_MEMDIE to the rest of the thread group at all. More on that
> > below
> > 
> > > thaw_processes has already been defined in kernel/power/process.c.
> > > Would it be better to use thaw_process instead?
> > 
> > Sorry I meant thaw_process as thaw_processes is handling all the
> > processes.
> > 
> > > 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.
> > 
> > And that is the reason why it shouldn't be doing that. It should thaw
> > the whole thread group. That's it.
> >  
> > > If the TIF_MEMDIE flag of a thread is not set, the thread cannot be thawed
> > > regardless of the cgroup state.
> > 
> > Why would that be the case. TIF_MEMDIE should only denote the victim
> > should be able to access memory reserves. Why the whole thread group
> > needs that? While more threads could be caught in the allocation path
> > this is a sort of boost at best. It cannot guarantee any forward
> > progress and we have kept marking only the first thread that way without
> > any issues.
> 
> When a process is frozen, all its threads enter __refrigerator() (in kernel/freezer.c). 
> When __thaw_task is called, the threads are woken up and check the freezing(current) 
> state (in __refrigerator). The freezing check is implemented via freezing_slow_path. 
> When TIF_MEMDIE is set for a thread, freezing_slow_path will return false, allowing 
> the thread to exit the infinite loop in __refrigerator(), and thus the thread will 
> be thawed.
> 
> The following code can explain how TIF_MEMDIE works in thread thawing.
> __refrigerator
>   for (;;) {
>     freezing = freezing(current)
>       freezing_slow_path
>         if (test_tsk_thread_flag(p, TIF_MEMDIE))
>           return false;
>     if (!freezing)
>       break;
>     schedule();
>   }

OK, I see. We could deal with that by checking tsk_is_oom_victim()
instead of TIF_MEMDIE

> Since thread_info is not shared within a thread group, TIF_MEMDIE for each thread  
> must be set so that all threads can be thawed.

-- 
Michal Hocko
SUSE Labs