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

Thank you, this looks great. It seems that oom_reserves_allowed implies
that tsk_is_oom_victim is not always effective (in page_alloc.c).
I will check it.