Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1409091 > unrolled thread

[PATCH 5/6] mm, oom: kill all tasks sharing the mm

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 5/6] mm, oom: kill all tasks sharing the mm Michal Hocko <mhocko@kernel.org> - 2016-05-30 15:10 +0200
    Re: [PATCH 5/6] mm, oom: kill all tasks sharing the mm Oleg Nesterov <oleg@redhat.com> - 2016-05-30 20:20 +0200
      Re: [PATCH 5/6] mm, oom: kill all tasks sharing the mm Michal Hocko <mhocko@kernel.org> - 2016-05-31 09:50 +0200
        Re: [PATCH 5/6] mm, oom: kill all tasks sharing the mm Oleg Nesterov <oleg@redhat.com> - 2016-05-31 23:50 +0200

#1409091 — [PATCH 5/6] mm, oom: kill all tasks sharing the mm

From: Michal Hocko <mhocko@suse.com>

Currently oom_kill_process skips both the oom reaper and SIG_KILL if a
process sharing the same mm is unkillable via OOM_ADJUST_MIN. After "mm,
oom_adj: make sure processes sharing mm have same view of oom_score_adj"
all such processes are sharing the same value so we shouldn't see such a
task at all (oom_badness would rule them out).

We can still encounter oom disabled vforked task which has to be killed
as well if we want to have other tasks sharing the mm reapable
because it can access the memory before doing exec. Killing such a task
should be acceptable because it is highly unlikely it has done anything
useful because it cannot modify any memory before it calls exec. An
alternative would be to keep the task alive and skip the oom reaper and
risk all the weird corner cases where the OOM killer cannot make forward
progress because the oom victim hung somewhere on the way to exit.

There is a potential race where we kill the oom disabled task which is
highly unlikely but possible. It would happen if __set_oom_adj raced
with select_bad_process and then it is OK to consider the old value or
with fork when it should be acceptable as well.
Let's add a little note to the log so that people would tell us that
this really happens in the real life and it matters.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 92bc8c3ec97b..d296f4467500 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -852,8 +852,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 			continue;
 		if (same_thread_group(p, victim))
 			continue;
-		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||
-		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
+		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p)) {
 			/*
 			 * We cannot use oom_reaper for the mm shared by this
 			 * process because it wouldn't get killed and so the
@@ -862,6 +861,11 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 			can_oom_reap = false;
 			continue;
 		}
+		if (p->signal->oom_score_adj == OOM_ADJUST_MIN)
+			pr_warn("%s pid=%d shares mm with oom disabled %s pid=%d. Seems like misconfiguration, killing anyway!"
+					" Report at linux-mm@kvack.org\n",
+					victim->comm, task_pid_nr(victim),
+					p->comm, task_pid_nr(p));
 		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 	}
 	rcu_read_unlock();
-- 
2.8.1

[toc] | [next] | [standalone]

#1409309

On 05/30, Michal Hocko wrote:
>
> @@ -852,8 +852,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  			continue;
>  		if (same_thread_group(p, victim))
>  			continue;
> -		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||
> -		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
> +		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p)) {
>  			/*
>  			 * We cannot use oom_reaper for the mm shared by this
>  			 * process because it wouldn't get killed and so the
> @@ -862,6 +861,11 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  			can_oom_reap = false;
>  			continue;
>  		}
> +		if (p->signal->oom_score_adj == OOM_ADJUST_MIN)
> +			pr_warn("%s pid=%d shares mm with oom disabled %s pid=%d. Seems like misconfiguration, killing anyway!"
> +					" Report at linux-mm@kvack.org\n",
> +					victim->comm, task_pid_nr(victim),
> +					p->comm, task_pid_nr(p));

Oh, yes, I personally do agree ;)

perhaps the is_global_init() == T case needs a warning too? the previous changes
take care about vfork() from /sbin/init, so the only reason we can see it true
is that /sbin/init shares the memory with a memory hog... Nevermind, forget.

This is a bit off-topic, but perhaps we can also change the PF_KTHREAD check later.
Of course we should not try to kill this kthread, but can_oom_reap can be true in
this case. A kernel thread which does use_mm() should handle the errors correctly
if (say) get_user() fails because we unmap the memory.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1409945

On Mon 30-05-16 20:18:16, Oleg Nesterov wrote:
> On 05/30, Michal Hocko wrote:
> >
> > @@ -852,8 +852,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  			continue;
> >  		if (same_thread_group(p, victim))
> >  			continue;
> > -		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p) ||
> > -		    p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) {
> > +		if (unlikely(p->flags & PF_KTHREAD) || is_global_init(p)) {
> >  			/*
> >  			 * We cannot use oom_reaper for the mm shared by this
> >  			 * process because it wouldn't get killed and so the
> > @@ -862,6 +861,11 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  			can_oom_reap = false;
> >  			continue;
> >  		}
> > +		if (p->signal->oom_score_adj == OOM_ADJUST_MIN)
> > +			pr_warn("%s pid=%d shares mm with oom disabled %s pid=%d. Seems like misconfiguration, killing anyway!"
> > +					" Report at linux-mm@kvack.org\n",
> > +					victim->comm, task_pid_nr(victim),
> > +					p->comm, task_pid_nr(p));
> 
> Oh, yes, I personally do agree ;)
> 
> perhaps the is_global_init() == T case needs a warning too? the previous changes
> take care about vfork() from /sbin/init, so the only reason we can see it true
> is that /sbin/init shares the memory with a memory hog... Nevermind, forget.

I have another two patches waiting for this to settle and one of them
adds a warning to that path.

> This is a bit off-topic, but perhaps we can also change the PF_KTHREAD check later.
> Of course we should not try to kill this kthread, but can_oom_reap can be true in
> this case. A kernel thread which does use_mm() should handle the errors correctly
> if (say) get_user() fails because we unmap the memory.

I was worried that the kernel thread would see a zero page so this could
lead to a data corruption.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1410652

On 05/31, Michal Hocko wrote:
>
> On Mon 30-05-16 20:18:16, Oleg Nesterov wrote:
> >
> > perhaps the is_global_init() == T case needs a warning too? the previous changes
> > take care about vfork() from /sbin/init, so the only reason we can see it true
> > is that /sbin/init shares the memory with a memory hog... Nevermind, forget.
>
> I have another two patches waiting for this to settle and one of them
> adds a warning to that path.

Good,

> > This is a bit off-topic, but perhaps we can also change the PF_KTHREAD check later.
> > Of course we should not try to kill this kthread, but can_oom_reap can be true in
> > this case. A kernel thread which does use_mm() should handle the errors correctly
> > if (say) get_user() fails because we unmap the memory.
>
> I was worried that the kernel thread would see a zero page so this could
> lead to a data corruption.

We can't avoid this anyway. use_mm(victim->mm) can be called after we decide to kill
the victim.

So I think that we should always ignore kthreads, and in task_will_free_mem() too.

But let me repeat, I agree we should discuss this later, I am not trying to suggest
this change right now.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web