[-v2,2/5] OOM: thaw the OOM victim if it is frozen

Commit Message

Michal Hocko Dec. 5, 2014, 4:41 p.m. UTC

oom_kill_process only sets TIF_MEMDIE flag and sends a signal to the
victim. This is basically noop when the task is frozen though because
the task sleeps in uninterruptible sleep. The victim is eventually
thawed later when oom_scan_process_thread meets the task again in a
later OOM invocation so the OOM killer doesn't live lock. But this is
less than optimal. Let's add the frozen check and thaw the task right
before we send SIGKILL to the victim.

The check and thawing in oom_scan_process_thread has to stay because the
task might got access to memory reserves even without an explicit
SIGKILL from oom_kill_process (e.g. it already has fatal signal pending
or it is exiting already).

Signed-off-by: Michal Hocko <[e-mail]>
---
 mm/oom_kill.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tejun Heo Dec. 6, 2014, 1:06 p.m. UTC | #1

Hello,

On Fri, Dec 05, 2014 at 05:41:44PM +0100, Michal Hocko wrote:
> oom_kill_process only sets TIF_MEMDIE flag and sends a signal to the
> victim. This is basically noop when the task is frozen though because
> the task sleeps in uninterruptible sleep. The victim is eventually
> thawed later when oom_scan_process_thread meets the task again in a
> later OOM invocation so the OOM killer doesn't live lock. But this is
> less than optimal. Let's add the frozen check and thaw the task right
> before we send SIGKILL to the victim.
> 
> The check and thawing in oom_scan_process_thread has to stay because the
> task might got access to memory reserves even without an explicit
> SIGKILL from oom_kill_process (e.g. it already has fatal signal pending
> or it is exiting already).

How else would a task get TIF_MEMDIE?  If there are other paths which
set TIF_MEMDIE, the right thing to do is creating a function which
thaws / wakes up the target task and use it there too.  Please
interlock these things properly from the get-go instead of scattering
these things around.

> @@ -545,6 +545,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  	rcu_read_unlock();
>  
>  	mark_tsk_oom_victim(victim);
> +	if (frozen(victim))
> +		__thaw_task(victim);

The frozen() test here is racy.  Always calling __thaw_task() wouldn't
be.  You can argue that being racy here is okay because the later
scanning would find it but why complicate things like that?  Just
properly interlock each instance and be done with it.

Thanks.

Michal Hocko Dec. 7, 2014, 10:24 a.m. UTC | #2

On Sat 06-12-14 08:06:57, Tejun Heo wrote:
> Hello,
> 
> On Fri, Dec 05, 2014 at 05:41:44PM +0100, Michal Hocko wrote:
> > oom_kill_process only sets TIF_MEMDIE flag and sends a signal to the
> > victim. This is basically noop when the task is frozen though because
> > the task sleeps in uninterruptible sleep. The victim is eventually
> > thawed later when oom_scan_process_thread meets the task again in a
> > later OOM invocation so the OOM killer doesn't live lock. But this is
> > less than optimal. Let's add the frozen check and thaw the task right
> > before we send SIGKILL to the victim.
> > 
> > The check and thawing in oom_scan_process_thread has to stay because the
> > task might got access to memory reserves even without an explicit
> > SIGKILL from oom_kill_process (e.g. it already has fatal signal pending
> > or it is exiting already).
> 
> How else would a task get TIF_MEMDIE?  If there are other paths which
> set TIF_MEMDIE, the right thing to do is creating a function which
> thaws / wakes up the target task and use it there too.  Please
> interlock these things properly from the get-go instead of scattering
> these things around.

See __out_of_memory which sets TIF_MEMDIE on current when it is exiting
or has fatal signals pending. This task cannot be frozen obviously.

> > @@ -545,6 +545,8 @@ void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> >  	rcu_read_unlock();
> >  
> >  	mark_tsk_oom_victim(victim);
> > +	if (frozen(victim))
> > +		__thaw_task(victim);
> 
> The frozen() test here is racy.  Always calling __thaw_task() wouldn't
> be.  You can argue that being racy here is okay because the later
> scanning would find it but why complicate things like that?  Just
> properly interlock each instance and be done with it.

OK, changed. I didn't realize that __thaw_task does the check already
and was following what we have in oom_scan_process_thread. Removed the
check from that one as well.

Patch

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index c75b37d59a32..8874058d62db 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -545,6 +545,8 @@  void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 	rcu_read_unlock();
 
 	mark_tsk_oom_victim(victim);
+	if (frozen(victim))
+		__thaw_task(victim);
 	do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
 	put_task_struct(victim);
 }