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

Groups > linux.kernel > #1336178 > unrolled thread

[PATCH 6/5] oom, oom_reaper: disable oom_reaper for

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 6/5] oom, oom_reaper: disable oom_reaper for Michal Hocko <mhocko@kernel.org> - 2016-02-17 10:50 +0100
    Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-02-17 11:50 +0100
      Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for Michal Hocko <mhocko@kernel.org> - 2016-02-17 12:40 +0100
    Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for Michal Hocko <mhocko@kernel.org> - 2016-02-19 19:40 +0100
      Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for oom_kill_allocating_task Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-02-20 03:40 +0100
        Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for  oom_kill_allocating_task Michal Hocko <mhocko@kernel.org> - 2016-02-22 10:50 +0100

#1336178 — [PATCH 6/5] oom, oom_reaper: disable oom_reaper for

Hi Andrew,
although this can be folded into patch 5
(mm-oom_reaper-implement-oom-victims-queuing.patch) I think it would be
better to have it separate and revert after we sort out the proper
oom_kill_allocating_task behavior or handle exclusion at oom_reaper
level.

Thanks!
---
From 7d8c953994f97fb38a8d71b53c06ecf8418616e9 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 17 Feb 2016 10:40:41 +0100
Subject: [PATCH] oom, oom_reaper: disable oom_reaper for
 oom_kill_allocating_task

Tetsuo has reported that oom_kill_allocating_task=1 will cause
oom_reaper_list corruption because oom_kill_process doesn't follow
standard OOM exclusion (aka ignores TIF_MEMDIE) and allows to enqueue
the same task multiple times - e.g. by sacrificing the same child
multiple times. Let's workaround this issue for now until we decide
how to handle oom_kill_allocating_task properly (should it sacrifice
children at all?) or come up with some other protection.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7e9953a64489..078e07ec0906 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -678,7 +678,14 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	unsigned int victim_points = 0;
 	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
 					      DEFAULT_RATELIMIT_BURST);
-	bool can_oom_reap = true;
+	bool can_oom_reap;
+
+	/*
+	 * XXX: oom_kill_allocating_task doesn't follow normal OOM exclusion
+	 * and so the same task might enter oom_kill_process which oom_reaper
+	 * cannot handle currently.
+	 */
+	can_oom_reap = !sysctl_oom_kill_allocating_task;
 
 	/*
 	 * If the task is already exiting, don't alarm the sysadmin or kill
-- 
2.7.0

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1336245

Michal Hocko wrote:
> Hi Andrew,
> although this can be folded into patch 5
> (mm-oom_reaper-implement-oom-victims-queuing.patch) I think it would be
> better to have it separate and revert after we sort out the proper
> oom_kill_allocating_task behavior or handle exclusion at oom_reaper
> level.

What a rough workaround. sysctl_oom_kill_allocating_task == 1 does not
always mean we must skip OOM reaper, for OOM-unkillable callers take
sysctl_oom_kill_allocating_task == 0 path.

I've just posted a patchset which allows you to merge the OOM reaper
without correcting problems found in "[PATCH 3/5] oom: clear TIF_MEMDIE
after oom_reaper managed to unmap the address space" and "[PATCH 5/5]
mm, oom_reaper: implement OOM victims queuing".

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

#1336275

On Wed 17-02-16 19:41:53, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Hi Andrew,
> > although this can be folded into patch 5
> > (mm-oom_reaper-implement-oom-victims-queuing.patch) I think it would be
> > better to have it separate and revert after we sort out the proper
> > oom_kill_allocating_task behavior or handle exclusion at oom_reaper
> > level.
> 
> What a rough workaround. sysctl_oom_kill_allocating_task == 1 does not
> always mean we must skip OOM reaper, for OOM-unkillable callers take
> sysctl_oom_kill_allocating_task == 0 path.

Yes it is indeed rough but also shouldn't add new issues. I consider
oom_kill_allocating_task as a borderline which can be sorted out later.
So while I do not like workarounds like this in general I would rather
go with obvious code first before going for more complex solutions.

> I've just posted a patchset which allows you to merge the OOM reaper
> without correcting problems found in "[PATCH 3/5] oom: clear TIF_MEMDIE
> after oom_reaper managed to unmap the address space" and "[PATCH 5/5]
> mm, oom_reaper: implement OOM victims queuing".

I will try to look at your patches but the series seems unnecessarily
heavy to be a pre-requisite for the oom_reaper.

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1338407

On Wed 17-02-16 10:48:55, Michal Hocko wrote:
> Hi Andrew,
> although this can be folded into patch 5
> (mm-oom_reaper-implement-oom-victims-queuing.patch) I think it would be
> better to have it separate and revert after we sort out the proper
> oom_kill_allocating_task behavior or handle exclusion at oom_reaper
> level.

An alternative would be something like the following. It is definitely
less hackish but it steals one bit in mm->flags. We do not seem to be
in shortage there now but who knows. Does this sound better? Later
changes might even consider the flag for the victim selection and ignore
those which already have the flag set. But I didn't think about it more
to form a patch yet.
---
From 8b17e66a70edac65ecd6df411a675cf3d840a9fe Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 17 Feb 2016 10:40:41 +0100
Subject: [PATCH] oom, oom_reaper: disable oom_reaper for
 oom_kill_allocating_task

Tetsuo has reported that oom_kill_allocating_task=1 will cause
oom_reaper_list corruption because oom_kill_process doesn't follow
standard OOM exclusion (aka ignores TIF_MEMDIE) and allows to enqueue
the same task multiple times - e.g. by sacrificing the same child
multiple times.

This patch fixes the issue by introducing a new MMF_OOM_KILLED mm flag
which is set in oom_kill_process atomically and oom reaper is disabled
if the flag was already set.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/sched.h | 2 ++
 mm/oom_kill.c         | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c25996c336de..0552cd5696c2 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -509,6 +509,8 @@ static inline int get_dumpable(struct mm_struct *mm)
 #define MMF_HAS_UPROBES		19	/* has uprobes */
 #define MMF_RECALC_UPROBES	20	/* MMF_HAS_UPROBES can be wrong */
 
+#define MMF_OOM_KILLED		21	/* OOM killer has chosen this mm */
+
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
 struct sighand_struct {
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7e9953a64489..32ce05b1aa10 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -678,7 +678,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	unsigned int victim_points = 0;
 	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
 					      DEFAULT_RATELIMIT_BURST);
-	bool can_oom_reap = true;
+	bool can_oom_reap;
 
 	/*
 	 * If the task is already exiting, don't alarm the sysadmin or kill
@@ -740,6 +740,10 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	/* Get a reference to safely compare mm after task_unlock(victim) */
 	mm = victim->mm;
 	atomic_inc(&mm->mm_count);
+
+	/* Make sure we do not try to oom reap the mm multiple times */
+	can_oom_reap = !test_and_set_bit(MMF_OOM_KILLED, &mm->flags);
+
 	/*
 	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
 	 * the OOM victim from depleting the memory reserves from the user
-- 
2.7.0


-- 
Michal Hocko
SUSE Labs

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

#1338622 — Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for oom_kill_allocating_task

Michal Hocko wrote:
> On Wed 17-02-16 10:48:55, Michal Hocko wrote:
> > Hi Andrew,
> > although this can be folded into patch 5
> > (mm-oom_reaper-implement-oom-victims-queuing.patch) I think it would be
> > better to have it separate and revert after we sort out the proper
> > oom_kill_allocating_task behavior or handle exclusion at oom_reaper
> > level.
> 
> An alternative would be something like the following. It is definitely
> less hackish but it steals one bit in mm->flags. We do not seem to be
> in shortage there now but who knows. Does this sound better? Later
> changes might even consider the flag for the victim selection and ignore
> those which already have the flag set. But I didn't think about it more
> to form a patch yet.

This sounds better than "can_oom_reap = !sysctl_oom_kill_allocating_task;".

> @@ -740,6 +740,10 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	/* Get a reference to safely compare mm after task_unlock(victim) */
>  	mm = victim->mm;
>  	atomic_inc(&mm->mm_count);
> +
> +	/* Make sure we do not try to oom reap the mm multiple times */
> +	can_oom_reap = !test_and_set_bit(MMF_OOM_KILLED, &mm->flags);
> +
>  	/*
>  	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
>  	 * the OOM victim from depleting the memory reserves from the user

But as of this line we don't know whether this mm is reapable.

Shouldn't this be done like

  static void wake_oom_reaper(struct task_struct *tsk)
  {
          /* Make sure we do not try to oom reap the mm multiple times */
          if (!oom_reaper_th || !test_and_set_bit(MMF_OOM_KILLED, &mm->flags))
                  return;

          get_task_struct(tsk);

          spin_lock(&oom_reaper_lock);
          list_add(&tsk->oom_reaper_list, &oom_reaper_list);
          spin_unlock(&oom_reaper_lock);
          wake_up(&oom_reaper_wait);
  }

?

Moreover, why don't you do like

  struct mm_struct {
  	(...snipped...)
  	struct list_head oom_reaper_list;
  	(...snipped...)
  }

than

  struct task_struct {
  	(...snipped...)
  	struct list_head oom_reaper_list;
  	(...snipped...)
  }

so that we can update all ->oom_score_adj using this mm_struct for handling
crazy combo ( http://lkml.kernel.org/r/20160204163113.GF14425@dhcp22.suse.cz ) ?

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

#1339298 — Re: [PATCH 6/5] oom, oom_reaper: disable oom_reaper for oom_kill_allocating_task

On Sat 20-02-16 11:32:07, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 17-02-16 10:48:55, Michal Hocko wrote:
> > > Hi Andrew,
> > > although this can be folded into patch 5
> > > (mm-oom_reaper-implement-oom-victims-queuing.patch) I think it would be
> > > better to have it separate and revert after we sort out the proper
> > > oom_kill_allocating_task behavior or handle exclusion at oom_reaper
> > > level.
> > 
> > An alternative would be something like the following. It is definitely
> > less hackish but it steals one bit in mm->flags. We do not seem to be
> > in shortage there now but who knows. Does this sound better? Later
> > changes might even consider the flag for the victim selection and ignore
> > those which already have the flag set. But I didn't think about it more
> > to form a patch yet.
> 
> This sounds better than "can_oom_reap = !sysctl_oom_kill_allocating_task;".
> 
> > @@ -740,6 +740,10 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
> >  	/* Get a reference to safely compare mm after task_unlock(victim) */
> >  	mm = victim->mm;
> >  	atomic_inc(&mm->mm_count);
> > +
> > +	/* Make sure we do not try to oom reap the mm multiple times */
> > +	can_oom_reap = !test_and_set_bit(MMF_OOM_KILLED, &mm->flags);
> > +
> >  	/*
> >  	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
> >  	 * the OOM victim from depleting the memory reserves from the user
> 
> But as of this line we don't know whether this mm is reapable.

Which is not really important. We know that it is eligible only if the
mm wasn't a part of the OOM kill before. Later checks are, of course,
allowed to veto the default and disable the oom reaper.

> Shouldn't this be done like
> 
>   static void wake_oom_reaper(struct task_struct *tsk)
>   {
>           /* Make sure we do not try to oom reap the mm multiple times */
>           if (!oom_reaper_th || !test_and_set_bit(MMF_OOM_KILLED, &mm->flags))
>                   return;

We do not have the mm here. We have a task and would need the task_lock.
I find it much easier to evaluate mm while we still have it and we know
the task holding this mm will receive SIGKILL and TIF_MEMDIE.
 
>           get_task_struct(tsk);
> 
>           spin_lock(&oom_reaper_lock);
>           list_add(&tsk->oom_reaper_list, &oom_reaper_list);
>           spin_unlock(&oom_reaper_lock);
>           wake_up(&oom_reaper_wait);
>   }
> 
> ?
> 
> Moreover, why don't you do like
> 
>   struct mm_struct {
>   	(...snipped...)
>   	struct list_head oom_reaper_list;
>   	(...snipped...)
>   }

Because we would need to search all tasks sharing the same mm in order
to exit_oom_victim.

> than
> 
>   struct task_struct {
>   	(...snipped...)
>   	struct list_head oom_reaper_list;
>   	(...snipped...)
>   }
> 
> so that we can update all ->oom_score_adj using this mm_struct for handling
> crazy combo ( http://lkml.kernel.org/r/20160204163113.GF14425@dhcp22.suse.cz ) ?

I find it much easier to to simply skip over tasks with MMF_OOM_KILLED
when already selecting a victim. We won't need oom_score_adj games at
all. This needs a deeper evaluation though. I didn't get to it yet,
but the point of having MMF flag which is not oom_reaper specific
was to have it reusable in other contexts as well.

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web