[patch] oom: thaw threads if oom killed thread is frozen before deferring

[patch] oom: thaw threads if oom killed thread is frozen before deferring

[David Rientjes]

On Tue, 27 Sep 2011, Michal Hocko wrote:

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 626303b..c419a7e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -32,6 +32,7 @@
>  #include <linux/mempolicy.h>
>  #include <linux/security.h>
>  #include <linux/ptrace.h>
> +#include <linux/freezer.h>
>  
>  int sysctl_panic_on_oom;
>  int sysctl_oom_kill_allocating_task;
> @@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
>  				task_pid_nr(q), q->comm);
>  			task_unlock(q);
>  			force_sig(SIGKILL, q);
> +
> +			if (frozen(q))
> +				thaw_process(q);
>  		}
>  
>  	set_tsk_thread_flag(p, TIF_MEMDIE);
>  	force_sig(SIGKILL, p);
> +	if (frozen(p))
> +		thaw_process(p);
>  
>  	return 0;
>  }

Also needs this...


oom: thaw threads if oom killed thread is frozen before deferring

If a thread has been oom killed and is frozen, thaw it before returning
to the page allocator.  Otherwise, it can stay frozen indefinitely and
no memory will be freed.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/oom_kill.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -318,8 +318,11 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		 * blocked waiting for another task which itself is waiting
 		 * for memory. Is there a better alternative?
 		 */
-		if (test_tsk_thread_flag(p, TIF_MEMDIE))
+		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
+			if (unlikely(frozen(p)))
+				thaw_process(p);
 			return ERR_PTR(-1UL);
+		}
 		if (!p->mm)
 			continue;
 

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Tue 27-09-11 11:35:04, David Rientjes wrote:
> On Tue, 27 Sep 2011, Michal Hocko wrote:
> 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 626303b..c419a7e 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/mempolicy.h>
> >  #include <linux/security.h>
> >  #include <linux/ptrace.h>
> > +#include <linux/freezer.h>
> >  
> >  int sysctl_panic_on_oom;
> >  int sysctl_oom_kill_allocating_task;
> > @@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
> >  				task_pid_nr(q), q->comm);
> >  			task_unlock(q);
> >  			force_sig(SIGKILL, q);
> > +
> > +			if (frozen(q))
> > +				thaw_process(q);
> >  		}
> >  
> >  	set_tsk_thread_flag(p, TIF_MEMDIE);
> >  	force_sig(SIGKILL, p);
> > +	if (frozen(p))
> > +		thaw_process(p);
> >  
> >  	return 0;
> >  }
> 
> Also needs this...
> 
> 
> oom: thaw threads if oom killed thread is frozen before deferring
> 
> If a thread has been oom killed and is frozen, thaw it before returning
> to the page allocator.  Otherwise, it can stay frozen indefinitely and
> no memory will be freed.

OK, I can see the race now:
oom_kill_task				refrigerator
  set_tsk_thread_flag(p, TIF_MEMDIE);
  force_sig(SIGKILL, p);
  if (frozen(p))
  	thaw_process(p)
					  frozen_process();
					  [...]
					  if (!frozen(current))
					  	break;
					  schedule();

select_bad_process
  [...]
  if (test_tsk_thread_flag(p, TIF_MEMDIE))
	  return ERR_PTR(-1UL);

So we either have to make sure that TIF_MEMDIE task is not frozen in
select_bad_process (your patch) or check for fatal_signal_pending
in refrigerator before we schedule and break out of the loop. Maybe the
later one is safer? Rafael?

Anyway this looks good. I guess it would be good to fold it into the
patch.

> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/oom_kill.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -318,8 +318,11 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
>  		 * blocked waiting for another task which itself is waiting
>  		 * for memory. Is there a better alternative?
>  		 */
> -		if (test_tsk_thread_flag(p, TIF_MEMDIE))
> +		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
> +			if (unlikely(frozen(p)))
> +				thaw_process(p);
>  			return ERR_PTR(-1UL);
> +		}
>  		if (!p->mm)
>  			continue;
>  
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Wed 28-09-11 12:44:45, Michal Hocko wrote:
> On Tue 27-09-11 11:35:04, David Rientjes wrote:
> > On Tue, 27 Sep 2011, Michal Hocko wrote:
> > 
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 626303b..c419a7e 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -32,6 +32,7 @@
> > >  #include <linux/mempolicy.h>
> > >  #include <linux/security.h>
> > >  #include <linux/ptrace.h>
> > > +#include <linux/freezer.h>
> > >  
> > >  int sysctl_panic_on_oom;
> > >  int sysctl_oom_kill_allocating_task;
> > > @@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
> > >  				task_pid_nr(q), q->comm);
> > >  			task_unlock(q);
> > >  			force_sig(SIGKILL, q);
> > > +
> > > +			if (frozen(q))
> > > +				thaw_process(q);
> > >  		}
> > >  
> > >  	set_tsk_thread_flag(p, TIF_MEMDIE);
> > >  	force_sig(SIGKILL, p);
> > > +	if (frozen(p))
> > > +		thaw_process(p);
> > >  
> > >  	return 0;
> > >  }
> > 
> > Also needs this...
> > 
> > 
> > oom: thaw threads if oom killed thread is frozen before deferring
> > 
> > If a thread has been oom killed and is frozen, thaw it before returning
> > to the page allocator.  Otherwise, it can stay frozen indefinitely and
> > no memory will be freed.
> 
> OK, I can see the race now:
> oom_kill_task				refrigerator
>   set_tsk_thread_flag(p, TIF_MEMDIE);
>   force_sig(SIGKILL, p);
>   if (frozen(p))
>   	thaw_process(p)
> 					  frozen_process();
> 					  [...]
> 					  if (!frozen(current))
> 					  	break;
> 					  schedule();
> 
> select_bad_process
>   [...]
>   if (test_tsk_thread_flag(p, TIF_MEMDIE))
> 	  return ERR_PTR(-1UL);
> 
> So we either have to make sure that TIF_MEMDIE task is not frozen in
> select_bad_process (your patch) or check for fatal_signal_pending
> in refrigerator before we schedule and break out of the loop. Maybe the
> later one is safer? Rafael?

What about this?
---
>From 2c9d15f19ae9b5e8f2497b41c1718782bc65e1e7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Thu, 29 Sep 2011 13:45:22 +0200
Subject: [PATCH] freezer: Get out of refrigerator if fatal signals are
 pending

We should make sure that the current task doesn't enter refrigerator if
it has fatal signals pending because it should get to the signals
processing as soon as possible.

This closes a possible race when OOM killer selects a task which is
about to enter the fridge but it is not set as frozen yet. This will
lead to a livelock because select_bad_process would skip that task due
to TIF_MEMDIE set for the process but there is no chance for further
process.
oom_kill_task                           refrigerator
  set_tsk_thread_flag(p, TIF_MEMDIE);
  force_sig(SIGKILL, p);
  if (frozen(p))
        thaw_process(p)
                                          frozen_process();
                                          [...]
                                          if (!frozen(current))
                                                break;
                                          schedule();

select_bad_process
  [...]
  if (test_tsk_thread_flag(p, TIF_MEMDIE))
          return ERR_PTR(-1UL);

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/freezer.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 7b01de9..74b8434 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -48,6 +48,10 @@ void refrigerator(void)
 	current->flags |= PF_FREEZING;
 
 	for (;;) {
+		if (fatal_signal_pending(current)) {
+			current->flags &= ~PF_FROZEN;
+			break;
+		}
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
-- 
1.7.6.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Oleg Nesterov]

On 09/29, Michal Hocko wrote:
>
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -48,6 +48,10 @@ void refrigerator(void)
>  	current->flags |= PF_FREEZING;
>  
>  	for (;;) {
> +		if (fatal_signal_pending(current)) {
> +			current->flags &= ~PF_FROZEN;

We can't do this.

If PF_FROZEN was set, we must not modify current->flags, this can
race with, say, thaw_process().

OK, we can take task_lock(), but this doesn't close other races.
Say, a SIGKILL'ed task can do try_to_freeze(). Perhaps we should
simply call thaw_process() unconditionally, this also clears
TIF_FREEZE. Or check freezing() || frozen(). Afacis this solves
the race you described.

But of course this can't help if freeze_task() is called later.
May be freezable() should check TIF_MEMDIE...

Oleg.

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Thu 29-09-11 14:05:17, Oleg Nesterov wrote:
> On 09/29, Michal Hocko wrote:
> >
> > --- a/kernel/freezer.c
> > +++ b/kernel/freezer.c
> > @@ -48,6 +48,10 @@ void refrigerator(void)
> >  	current->flags |= PF_FREEZING;
> >  
> >  	for (;;) {
> > +		if (fatal_signal_pending(current)) {
> > +			current->flags &= ~PF_FROZEN;
> 
> We can't do this.
> 
> If PF_FROZEN was set, we must not modify current->flags, this can
> race with, say, thaw_process().

OK, I see.

> 
> OK, we can take task_lock(), but this doesn't close other races.
> Say, a SIGKILL'ed task can do try_to_freeze(). Perhaps we should
> simply call thaw_process() unconditionally, this also clears
> TIF_FREEZE. 
> Or check freezing() || frozen(). Afacis this solves
> the race you described.

Sounds reasonable.

> 
> But of course this can't help if freeze_task() is called later.
> May be freezable() should check TIF_MEMDIE...

Wouldn't it be easier to ignore try_to_freeze when fatal signals are
pending in get_signal_to_deliver? This would mean that we wouldn't get
back to refrigerator just to get out of it.
What about the patch bellow?

> 
> Oleg.

Thanks!

---
>From 3c6c4b4f1a21c34ea1db76b765ce671ca97d9c3e Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Thu, 29 Sep 2011 13:45:22 +0200
Subject: [PATCH] freezer: Get out of refrigerator if fatal signals are
 pending

We should make sure that the current task doesn't enter refrigerator if
it has fatal signals pending because it should get to the signals
processing as soon as possible. Thaw the process if it is either
freezing or still frozen to prevent from races with thaw_process.

This closes a possible race when OOM killer selects a task which is
about to enter the fridge but it is not set as frozen yet. This will
lead to a livelock because select_bad_process would skip that task due
to TIF_MEMDIE set for the process but there is no chance for further
process.
oom_kill_task                           refrigerator
  set_tsk_thread_flag(p, TIF_MEMDIE);
  force_sig(SIGKILL, p);
  if (frozen(p))
        thaw_process(p)
                                          frozen_process();
                                          [...]
                                          if (!frozen(current))
                                                break;
                                          schedule();

select_bad_process
  [...]
  if (test_tsk_thread_flag(p, TIF_MEMDIE))
          return ERR_PTR(-1UL);

Let's skip try_to_freeze in get_signal_to_deliver if fatal signals are
pending to make sure that we will not somebody will keep us looping
between refrigerator and get_signal_to_deliver for ever.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/freezer.c |    5 +++++
 kernel/signal.c  |    4 +++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 7b01de9..0531661 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -48,6 +48,11 @@ void refrigerator(void)
 	current->flags |= PF_FREEZING;
 
 	for (;;) {
+		if (fatal_signal_pending(current)) {
+			if (freezing(current) || frozen(current))
+				thaw_process(current);
+			break;
+		}
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
diff --git a/kernel/signal.c b/kernel/signal.c
index 291c970..bc97a6a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2147,8 +2147,10 @@ relock:
 	 * While in TASK_STOPPED, we were considered "frozen enough".
 	 * Now that we woke up, it's crucial if we're supposed to be
 	 * frozen that we freeze now before running anything substantial.
+	 * Let's ignore the freezing request if we are about to die anyway.
 	 */
-	try_to_freeze();
+	if (!fatal_signal_pending(curret))
+		try_to_freeze();
 
 	spin_lock_irq(&sighand->siglock);
 	/*
-- 
1.7.6.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Thu 29-09-11 15:02:04, Michal Hocko wrote:
[...]
> From 3c6c4b4f1a21c34ea1db76b765ce671ca97d9c3e Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Thu, 29 Sep 2011 13:45:22 +0200
> Subject: [PATCH] freezer: Get out of refrigerator if fatal signals are
>  pending
> 
> We should make sure that the current task doesn't enter refrigerator if
> it has fatal signals pending because it should get to the signals
> processing as soon as possible. Thaw the process if it is either
> freezing or still frozen to prevent from races with thaw_process.
> 
> This closes a possible race when OOM killer selects a task which is
> about to enter the fridge but it is not set as frozen yet. This will
> lead to a livelock because select_bad_process would skip that task due
> to TIF_MEMDIE set for the process but there is no chance for further
> process.
> oom_kill_task                           refrigerator
>   set_tsk_thread_flag(p, TIF_MEMDIE);
>   force_sig(SIGKILL, p);
>   if (frozen(p))
>         thaw_process(p)
>                                           frozen_process();
>                                           [...]
>                                           if (!frozen(current))
>                                                 break;
>                                           schedule();
> 
> select_bad_process
>   [...]
>   if (test_tsk_thread_flag(p, TIF_MEMDIE))
>           return ERR_PTR(-1UL);
> 
> Let's skip try_to_freeze in get_signal_to_deliver if fatal signals are
> pending to make sure that we will not somebody will keep us looping
> between refrigerator and get_signal_to_deliver for ever.

I have just read through the description again. I have rewritten it
several times and this is the messed up result. Sorry about that.
The endless loop is not possible as we will handle the fatal signal
after we get back from try_to_freeze and die.
It should read:

"
Let's skip try_to_freeze in get_signal_to_deliver if fatal signals are
pending to make sure that we will not get back to refrigerator again
just to get back immediately.
"

> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  kernel/freezer.c |    5 +++++
>  kernel/signal.c  |    4 +++-
>  2 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index 7b01de9..0531661 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -48,6 +48,11 @@ void refrigerator(void)
>  	current->flags |= PF_FREEZING;
>  
>  	for (;;) {
> +		if (fatal_signal_pending(current)) {
> +			if (freezing(current) || frozen(current))
> +				thaw_process(current);
> +			break;
> +		}
>  		set_current_state(TASK_UNINTERRUPTIBLE);
>  		if (!frozen(current))
>  			break;
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 291c970..bc97a6a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2147,8 +2147,10 @@ relock:
>  	 * While in TASK_STOPPED, we were considered "frozen enough".
>  	 * Now that we woke up, it's crucial if we're supposed to be
>  	 * frozen that we freeze now before running anything substantial.
> +	 * Let's ignore the freezing request if we are about to die anyway.
>  	 */
> -	try_to_freeze();
> +	if (!fatal_signal_pending(curret))
> +		try_to_freeze();
>  
>  	spin_lock_irq(&sighand->siglock);
>  	/*
> -- 
> 1.7.6.3
> 
> -- 
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9    
> Czech Republic

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Oleg Nesterov]

On 09/29, Michal Hocko wrote:
>
> On Thu 29-09-11 14:05:17, Oleg Nesterov wrote:
>
> > But of course this can't help if freeze_task() is called later.
> > May be freezable() should check TIF_MEMDIE...
>
> Wouldn't it be easier to ignore try_to_freeze when fatal signals are
> pending in get_signal_to_deliver?

Oh, I don't think so. For what? This doesn't close other races, and
in fact the fatal_signal_pending() this patch adds is itself racy,
SIGKILL can come in between.

> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -48,6 +48,11 @@ void refrigerator(void)
>  	current->flags |= PF_FREEZING;
>
>  	for (;;) {
> +		if (fatal_signal_pending(current)) {
> +			if (freezing(current) || frozen(current))
> +				thaw_process(current);

Ah, I didn't mean refrigerator() should check freezing/frozen.

I meant, oom_kill can do this before thaw thaw_process(), afaics
this should fix the particular race you described (but not others).

And. It is simply wrong to return from refrigerator() after we set
PF_FROZEN, this can fool try_to_freeze_tasks(). Sure, thaw_process()
from oom_kill is not nice too, but at least this is the special case,
we already have the problem.

Oleg.

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Thu 29-09-11 18:37:24, Oleg Nesterov wrote:
> On 09/29, Michal Hocko wrote:
> >
> > On Thu 29-09-11 14:05:17, Oleg Nesterov wrote:
> >
> > > But of course this can't help if freeze_task() is called later.
> > > May be freezable() should check TIF_MEMDIE...
> >
> > Wouldn't it be easier to ignore try_to_freeze when fatal signals are
> > pending in get_signal_to_deliver?
> 
> Oh, I don't think so. For what? This doesn't close other races, and
> in fact the fatal_signal_pending() this patch adds is itself racy,
> SIGKILL can come in between.

OK, I think I see your point. You mean that oom will send KILL after
both fatal_signal_pending in refrigerator and signal_pending check in
schedule, right?

> 
> > --- a/kernel/freezer.c
> > +++ b/kernel/freezer.c
> > @@ -48,6 +48,11 @@ void refrigerator(void)
> >  	current->flags |= PF_FREEZING;
> >
> >  	for (;;) {
> > +		if (fatal_signal_pending(current)) {
> > +			if (freezing(current) || frozen(current))
> > +				thaw_process(current);
> 
> Ah, I didn't mean refrigerator() should check freezing/frozen.
> 
> I meant, oom_kill can do this before thaw thaw_process(), afaics
> this should fix the particular race you described (but not others).

This is what the follow up fix from David is doing. Check frozen in
select_bad_process if the task is TIF_MEMDIE and thaw the process.

And it seems that the David's follow up fix is sufficient so let's leave
refrigerator alone.
Or am I still missing something?

> 
> And. It is simply wrong to return from refrigerator() after we set
> PF_FROZEN, this can fool try_to_freeze_tasks(). Sure, thaw_process()
> from oom_kill is not nice too, but at least this is the special case,
> we already have the problem.
> 
> Oleg.
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Tejun Heo]

Hello,

On Thu, Sep 29, 2011 at 08:00:21PM +0200, Michal Hocko wrote:
> > I meant, oom_kill can do this before thaw thaw_process(), afaics
> > this should fix the particular race you described (but not others).
> 
> This is what the follow up fix from David is doing. Check frozen in
> select_bad_process if the task is TIF_MEMDIE and thaw the process.
> 
> And it seems that the David's follow up fix is sufficient so let's leave
> refrigerator alone.
> Or am I still missing something?

With pending freezer changes, allowing TIF_MEMDIE tasks to exit
freezer by modifying freezing() shouldn't be difficult, which should
be race-free and much simpler than diddling with thaw_task().  How
urgent is this?  Can we wait for the next merge window?

Thanks.

-- 
tejun

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Thu 29-09-11 18:51:48, Tejun Heo wrote:
> Hello,
> 
> On Thu, Sep 29, 2011 at 08:00:21PM +0200, Michal Hocko wrote:
> > > I meant, oom_kill can do this before thaw thaw_process(), afaics
> > > this should fix the particular race you described (but not others).
> > 
> > This is what the follow up fix from David is doing. Check frozen in
> > select_bad_process if the task is TIF_MEMDIE and thaw the process.
> > 
> > And it seems that the David's follow up fix is sufficient so let's leave
> > refrigerator alone.
> > Or am I still missing something?
> 
> With pending freezer changes, allowing TIF_MEMDIE tasks to exit
> freezer by modifying freezing() shouldn't be difficult, which should
> be race-free and much simpler than diddling with thaw_task().  

Will the rework help with the initial problem of unkillable OOM selected
frozen tasks or it will just help with other races that might be present
with the patch? In other words will this work deprecate the 2 patches
sent earlier in this thread?

> How urgent is this?  Can we wait for the next merge window?

Yes, I think we can wait some more.

> 
> Thanks.
> 
> -- 
> tejun
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Tejun Heo]

Hello,

On Fri, Sep 30, 2011 at 09:41:25AM +0200, Michal Hocko wrote:
> > With pending freezer changes, allowing TIF_MEMDIE tasks to exit
> > freezer by modifying freezing() shouldn't be difficult, which should
> > be race-free and much simpler than diddling with thaw_task().  
> 
> Will the rework help with the initial problem of unkillable OOM selected
> frozen tasks or it will just help with other races that might be present
> with the patch? In other words will this work deprecate the 2 patches
> sent earlier in this thread?

I think it shouldn't be difficult to allow OOM-killing frozen tasks.
That should be good enough, right?

> > How urgent is this?  Can we wait for the next merge window?
> 
> Yes, I think we can wait some more.

I'm still processing rather large backlog.  I'll ping you back once I
sort out the pending freezer changes.

Thanks.

-- 
tejun

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Michal Hocko]

On Fri 30-09-11 00:46:41, Tejun Heo wrote:
> Hello,
> 
> On Fri, Sep 30, 2011 at 09:41:25AM +0200, Michal Hocko wrote:
> > > With pending freezer changes, allowing TIF_MEMDIE tasks to exit
> > > freezer by modifying freezing() shouldn't be difficult, which should
> > > be race-free and much simpler than diddling with thaw_task().  
> > 
> > Will the rework help with the initial problem of unkillable OOM selected
> > frozen tasks or it will just help with other races that might be present
> > with the patch? In other words will this work deprecate the 2 patches
> > sent earlier in this thread?
> 
> I think it shouldn't be difficult to allow OOM-killing frozen tasks.
> That should be good enough, right?

Yes, if you could just force_sig(SIGKILL, p) frozen task then it would
be ideal because we wouldn't have to call thaw_process from OOM path.

> 
> > > How urgent is this?  Can we wait for the next merge window?
> > 
> > Yes, I think we can wait some more.
> 
> I'm still processing rather large backlog.  I'll ping you back once I
> sort out the pending freezer changes.

You were on the CC quite early but it was quite late when I noticed that
I have accidentally used your kernel.org address. Just for reference the
original discussion started here: https://lkml.org/lkml/2011/8/23/45 and
this thread started here:
http://www.spinics.net/lists/linux-mm/msg24693.html

> 
> Thanks.
> 
> -- 
> tejun
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [patch] oom: thaw threads if oom killed thread is frozen before deferring

[Oleg Nesterov]

On 09/29, Michal Hocko wrote:
>
> On Thu 29-09-11 18:37:24, Oleg Nesterov wrote:
> >
> > Oh, I don't think so. For what? This doesn't close other races, and
> > in fact the fatal_signal_pending() this patch adds is itself racy,
> > SIGKILL can come in between.
>
> OK, I think I see your point. You mean that oom will send KILL after
> both fatal_signal_pending in refrigerator and signal_pending check in
> schedule, right?

No, schedule()->signal_pending_state(TASK_UNINTERRUPTIBLE) doesn't check
the signals. I simply meant

	if (fatal_signal_pending())
					// <--- SIGKILL from oom
		try_to_freeze();

> This is what the follow up fix from David is doing. Check frozen in
> select_bad_process if the task is TIF_MEMDIE and thaw the process.
>
> And it seems that the David's follow up fix is sufficient so let's leave
> refrigerator alone.

Agreed, afaics this should fix all races (although I didn't read the
whole discussion, perhaps I missed something else).

And in this case we do not even need to modify oom_kill_task/etc,
select_bad_process() will be called again and notice the frozen task
eventually. Afaics.

Or, as Tejun suggests, we can implement the race-free kill-even-if-frozen
later.

Oleg.

Re: [PATCH 2/2] oom: do not live lock on frozen tasks

[Andrew Morton]

On Tue, 27 Sep 2011 10:01:47 +0200
Michal Hocko <mhocko@suse.cz> wrote:

> Konstantin Khlebnikov has reported (https://lkml.org/lkml/2011/8/23/45)
> that OOM can end up in a live lock if select_bad_process picks up a frozen
> task.
> Unfortunately we cannot mark such processes as unkillable to ignore them
> because we could panic the system even though there is a chance that
> somebody could thaw the process so we can make a forward process (e.g. a
> process from another cpuset or with a different nodemask).
> 
> Let's thaw an OOM selected frozen process right after we've sent fatal
> signal from oom_kill_task.
> Thawing is safe if the frozen task doesn't access any suspended device
> (e.g. by ioctl) on the way out to the userspace where we handle the
> signal and die. Note, we are not interested in the kernel threads because
> they are not oom killable.
> 
> Accessing suspended devices by a userspace processes shouldn't be an
> issue because devices are suspended only after userspace is already
> frozen and oom is disabled at that time.
> 
> Other than that userspace accesses the fridge only from the
> signal handling routines so we are able to handle SIGKILL without any
> negative side effects or we always check for pending signals after
> we return from try_to_freeze (e.g. in lguest).
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Reported-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Acked-by: David Rientjes <rientjes@google.com>
> ---
>  mm/oom_kill.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 626303b..c419a7e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -32,6 +32,7 @@
>  #include <linux/mempolicy.h>
>  #include <linux/security.h>
>  #include <linux/ptrace.h>
> +#include <linux/freezer.h>
>  
>  int sysctl_panic_on_oom;
>  int sysctl_oom_kill_allocating_task;
> @@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
>  				task_pid_nr(q), q->comm);
>  			task_unlock(q);
>  			force_sig(SIGKILL, q);
> +
> +			if (frozen(q))
> +				thaw_process(q);
>  		}
>  
>  	set_tsk_thread_flag(p, TIF_MEMDIE);
>  	force_sig(SIGKILL, p);
> +	if (frozen(p))
> +		thaw_process(p);
>  
>  	return 0;
>  }

I'm not sure this is 1000% correct.  Perhaps there's a conceivable
window after the "if (frozen)" test where the task can flip itself into
the frozen state.

thaw_process() itself appears to be callable regardless of the frozen
state and will do the right thing under the right lock.  So this code
would be safer, correcter and slower if it unconditionally called
thaw_process().

I'm sure it doesn't matter though ;)

Re: [PATCH 2/2] oom: do not live lock on frozen tasks

[Michal Hocko]

On Fri 28-10-11 15:23:21, Andrew Morton wrote:
> On Tue, 27 Sep 2011 10:01:47 +0200
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > Konstantin Khlebnikov has reported (https://lkml.org/lkml/2011/8/23/45)
> > that OOM can end up in a live lock if select_bad_process picks up a frozen
> > task.
> > Unfortunately we cannot mark such processes as unkillable to ignore them
> > because we could panic the system even though there is a chance that
> > somebody could thaw the process so we can make a forward process (e.g. a
> > process from another cpuset or with a different nodemask).
> > 
> > Let's thaw an OOM selected frozen process right after we've sent fatal
> > signal from oom_kill_task.
> > Thawing is safe if the frozen task doesn't access any suspended device
> > (e.g. by ioctl) on the way out to the userspace where we handle the
> > signal and die. Note, we are not interested in the kernel threads because
> > they are not oom killable.
> > 
> > Accessing suspended devices by a userspace processes shouldn't be an
> > issue because devices are suspended only after userspace is already
> > frozen and oom is disabled at that time.
> > 
> > Other than that userspace accesses the fridge only from the
> > signal handling routines so we are able to handle SIGKILL without any
> > negative side effects or we always check for pending signals after
> > we return from try_to_freeze (e.g. in lguest).
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > Reported-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Acked-by: David Rientjes <rientjes@google.com>
> > ---
> >  mm/oom_kill.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 626303b..c419a7e 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/mempolicy.h>
> >  #include <linux/security.h>
> >  #include <linux/ptrace.h>
> > +#include <linux/freezer.h>
> >  
> >  int sysctl_panic_on_oom;
> >  int sysctl_oom_kill_allocating_task;
> > @@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
> >  				task_pid_nr(q), q->comm);
> >  			task_unlock(q);
> >  			force_sig(SIGKILL, q);
> > +
> > +			if (frozen(q))
> > +				thaw_process(q);
> >  		}
> >  
> >  	set_tsk_thread_flag(p, TIF_MEMDIE);
> >  	force_sig(SIGKILL, p);
> > +	if (frozen(p))
> > +		thaw_process(p);
> >  
> >  	return 0;
> >  }
> 
> I'm not sure this is 1000% correct.  Perhaps there's a conceivable
> window after the "if (frozen)" test where the task can flip itself into
> the frozen state.

Yes and David's patch
(oom-thaw-threads-if-oom-killed-thread-is-frozen-before-deferring.patch)
is much better in that regards. So we should go with the other patch.

> 
> thaw_process() itself appears to be callable regardless of the frozen
> state and will do the right thing under the right lock.  So this code
> would be safer, correcter and slower if it unconditionally called
> thaw_process().
> 
> I'm sure it doesn't matter though ;)
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 1/2] lguest: move process freezing before pending signals check

[Michal Hocko]

Hi Rusty,
what is the current state of this patch? Are you planning to push it for
3.2?

Thanks

On Tue 27-09-11 08:56:03, Michal Hocko wrote:
> run_guest tries to freeze the current process after it has handled
> pending interrupts and before it calls lguest_arch_run_guest.
> This doesn't work nicely if the task has been killed while being frozen
> and when we want to handle that signal as soon as possible.
> Let's move try_to_freeze before we check for pending signal so that we
> can get out of the loop as soon as possible.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/lguest/core.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/lguest/core.c b/drivers/lguest/core.c
> index 2535933..e7dda91 100644
> --- a/drivers/lguest/core.c
> +++ b/drivers/lguest/core.c
> @@ -232,6 +232,13 @@ int run_guest(struct lg_cpu *cpu, unsigned long __user *user)
>  			}
>  		}
>  
> +		/*
> +		 * All long-lived kernel loops need to check with this horrible
> +		 * thing called the freezer.  If the Host is trying to suspend,
> +		 * it stops us.
> +		 */
> +		try_to_freeze();
> +
>  		/* Check for signals */
>  		if (signal_pending(current))
>  			return -ERESTARTSYS;
> @@ -246,13 +253,6 @@ int run_guest(struct lg_cpu *cpu, unsigned long __user *user)
>  			try_deliver_interrupt(cpu, irq, more);
>  
>  		/*
> -		 * All long-lived kernel loops need to check with this horrible
> -		 * thing called the freezer.  If the Host is trying to suspend,
> -		 * it stops us.
> -		 */
> -		try_to_freeze();
> -
> -		/*
>  		 * Just make absolutely sure the Guest is still alive.  One of
>  		 * those hypercalls could have been fatal, for example.
>  		 */
> -- 
> 1.7.5.4
> 

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH 1/2] lguest: move process freezing before pending signals check

[Rusty Russell]

On Wed, 12 Oct 2011 08:55:04 +0200, Michal Hocko <mhocko@suse.cz> wrote:
> Hi Rusty,
> what is the current state of this patch? Are you planning to push it for
> 3.2?

Oh.  Having acked it, I assumed you'd push.  But I've put it in my queue
now.

Thanks,
Rusty.

Re: [PATCH 1/2] lguest: move process freezing before pending signals check

[Michal Hocko]

On Thu 13-10-11 10:27:07, Rusty Russell wrote:
> On Wed, 12 Oct 2011 08:55:04 +0200, Michal Hocko <mhocko@suse.cz> wrote:
> > Hi Rusty,
> > what is the current state of this patch? Are you planning to push it for
> > 3.2?
> 
> Oh.  Having acked it, I assumed you'd push.  But I've put it in my queue
> now.

Thanks

> 
> Thanks,
> Rusty.

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic