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

Groups > linux.kernel > #1336488 > unrolled thread

[PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable. Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-02-17 15:40 +0100
    Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-17 15:50 +0100
    Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they  are OOM-unkillable. David Rientjes <rientjes@google.com> - 2016-02-17 23:40 +0100
      Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-18 09:10 +0100
        Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable. Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-02-18 11:40 +0100
          Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-18 13:10 +0100
            Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-18 13:20 +0100
              Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable. Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-02-19 16:10 +0100
                Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are  OOM-unkillable. Michal Hocko <mhocko@kernel.org> - 2016-02-19 16:20 +0100

#1336488 — [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

oom_scan_process_thread() returns OOM_SCAN_SELECT when there is a
thread which returns oom_task_origin() == true. But it is possible
that such thread is marked as OOM-unkillable. In that case, the OOM
killer must not select such process.

Since it is meaningless to return OOM_SCAN_OK for OOM-unkillable
process because subsequent oom_badness() call will return 0, this
patch changes oom_scan_process_thread to return OOM_SCAN_CONTINUE
if that process is marked as OOM-unkillable (regardless of
oom_task_origin()).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Suggested-by: Michal Hocko <mhocko@kernel.org>
---
 mm/oom_kill.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7653055..cf87153 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -282,7 +282,7 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
 		if (!is_sysrq_oom(oc))
 			return OOM_SCAN_ABORT;
 	}
-	if (!task->mm)
+	if (!task->mm || task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
 		return OOM_SCAN_CONTINUE;
 
 	/*
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1336493 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Wed 17-02-16 23:31:00, Tetsuo Handa wrote:
> oom_scan_process_thread() returns OOM_SCAN_SELECT when there is a
> thread which returns oom_task_origin() == true. But it is possible
> that such thread is marked as OOM-unkillable. In that case, the OOM
> killer must not select such process.

As already pointed out swapoff or ksm run_store are the only users of
OOM_FLAG_ORIGIN and it would be insane to run them from an oom disabled
context. So I wouldn't care much about this part that much and consider
the patch to be more of a cleanup rather than a bug fix.

> Since it is meaningless to return OOM_SCAN_OK for OOM-unkillable
> process because subsequent oom_badness() call will return 0, this
> patch changes oom_scan_process_thread to return OOM_SCAN_CONTINUE
> if that process is marked as OOM-unkillable (regardless of
> oom_task_origin()).
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Suggested-by: Michal Hocko <mhocko@kernel.org>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/oom_kill.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7653055..cf87153 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -282,7 +282,7 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
>  		if (!is_sysrq_oom(oc))
>  			return OOM_SCAN_ABORT;
>  	}
> -	if (!task->mm)
> +	if (!task->mm || task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
>  		return OOM_SCAN_CONTINUE;
>  
>  	/*
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

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

#1336825 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Wed, 17 Feb 2016, Tetsuo Handa wrote:

> oom_scan_process_thread() returns OOM_SCAN_SELECT when there is a
> thread which returns oom_task_origin() == true. But it is possible
> that such thread is marked as OOM-unkillable. In that case, the OOM
> killer must not select such process.
> 
> Since it is meaningless to return OOM_SCAN_OK for OOM-unkillable
> process because subsequent oom_badness() call will return 0, this
> patch changes oom_scan_process_thread to return OOM_SCAN_CONTINUE
> if that process is marked as OOM-unkillable (regardless of
> oom_task_origin()).
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> ---
>  mm/oom_kill.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7653055..cf87153 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -282,7 +282,7 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
>  		if (!is_sysrq_oom(oc))
>  			return OOM_SCAN_ABORT;
>  	}
> -	if (!task->mm)
> +	if (!task->mm || task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
>  		return OOM_SCAN_CONTINUE;
>  
>  	/*

I'm getting multiple emails from you with the identical patch, something 
is definitely wacky in your toolchain.

Anyway, this is NACK'd since task->signal->oom_score_adj is checked under 
task_lock() for threads with memory attached, that's the purpose of 
finding the correct thread in oom_badness() and taking task_lock().  We 
aren't going to duplicate logic in several functions that all do the same 
thing.

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

#1337116 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Wed 17-02-16 14:31:54, David Rientjes wrote:
> On Wed, 17 Feb 2016, Tetsuo Handa wrote:
> 
> > oom_scan_process_thread() returns OOM_SCAN_SELECT when there is a
> > thread which returns oom_task_origin() == true. But it is possible
> > that such thread is marked as OOM-unkillable. In that case, the OOM
> > killer must not select such process.
> > 
> > Since it is meaningless to return OOM_SCAN_OK for OOM-unkillable
> > process because subsequent oom_badness() call will return 0, this
> > patch changes oom_scan_process_thread to return OOM_SCAN_CONTINUE
> > if that process is marked as OOM-unkillable (regardless of
> > oom_task_origin()).
> > 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Suggested-by: Michal Hocko <mhocko@kernel.org>
> > ---
> >  mm/oom_kill.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 7653055..cf87153 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -282,7 +282,7 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> >  		if (!is_sysrq_oom(oc))
> >  			return OOM_SCAN_ABORT;
> >  	}
> > -	if (!task->mm)
> > +	if (!task->mm || task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> >  		return OOM_SCAN_CONTINUE;
> >  
> >  	/*
> 
> I'm getting multiple emails from you with the identical patch, something 
> is definitely wacky in your toolchain.
> 
> Anyway, this is NACK'd since task->signal->oom_score_adj is checked under 
> task_lock() for threads with memory attached, that's the purpose of 
> finding the correct thread in oom_badness() and taking task_lock().  We 
> aren't going to duplicate logic in several functions that all do the same 
> thing.

Is the task_lock really necessary, though? E.g. oom_task_origin()
doesn't seem to depend on it for task->signal safety. If you are
referring to races with changing oom_score_adj does such a race matter
at all?

To me this looks like a reasonable cleanup because we _know_ that
OOM_SCORE_ADJ_MIN means OOM_SCAN_CONTINUE and do not really have to go
down to oom_badness to find that out. Or what am I missing?

-- 
Michal Hocko
SUSE Labs

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

#1337249

Michal Hocko wrote:
> On Wed 17-02-16 14:31:54, David Rientjes wrote:
> > On Wed, 17 Feb 2016, Tetsuo Handa wrote:
> > 
> > > oom_scan_process_thread() returns OOM_SCAN_SELECT when there is a
> > > thread which returns oom_task_origin() == true. But it is possible
> > > that such thread is marked as OOM-unkillable. In that case, the OOM
> > > killer must not select such process.
> > > 
> > > Since it is meaningless to return OOM_SCAN_OK for OOM-unkillable
> > > process because subsequent oom_badness() call will return 0, this
> > > patch changes oom_scan_process_thread to return OOM_SCAN_CONTINUE
> > > if that process is marked as OOM-unkillable (regardless of
> > > oom_task_origin()).
> > > 
> > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Suggested-by: Michal Hocko <mhocko@kernel.org>
> > > ---
> > >  mm/oom_kill.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 7653055..cf87153 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -282,7 +282,7 @@ enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
> > >  		if (!is_sysrq_oom(oc))
> > >  			return OOM_SCAN_ABORT;
> > >  	}
> > > -	if (!task->mm)
> > > +	if (!task->mm || task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > >  		return OOM_SCAN_CONTINUE;
> > >  
> > >  	/*
> > 
> > I'm getting multiple emails from you with the identical patch, something 
> > is definitely wacky in your toolchain.

Sorry, my operation error. I didn't know I can't do like below.

  git send-email --to="rcpt1 rcpt2" --cc="rcpt3 rcpt4 rcpt5" file.patch

Thus, I resent the identical patch. Not a toolchain problem.

> > 
> > Anyway, this is NACK'd since task->signal->oom_score_adj is checked under 
> > task_lock() for threads with memory attached, that's the purpose of 
> > finding the correct thread in oom_badness() and taking task_lock().  We 
> > aren't going to duplicate logic in several functions that all do the same 
> > thing.
> 
> Is the task_lock really necessary, though? E.g. oom_task_origin()
> doesn't seem to depend on it for task->signal safety. If you are
> referring to races with changing oom_score_adj does such a race matter
> at all?
> 
> To me this looks like a reasonable cleanup because we _know_ that
> OOM_SCORE_ADJ_MIN means OOM_SCAN_CONTINUE and do not really have to go
> down to oom_badness to find that out. Or what am I missing?
> 

That NACK will not matter if a draft patch shown bottom is acceptable.

I got a question about commit 9cbb78bb314360a8 "mm, memcg: introduce own
oom handler to iterate only over its own threads" while trying to kill
duplicated oom_unkillable_task() checks by merging oom_scan_process_thread()
into oom_badness().

Currently, oom_scan_process_thread() is doing

  if (oom_unkillable_task(p, NULL, oc->nodemask))
  	return OOM_SCAN_CONTINUE;

and oom_badness() is doing

  if (oom_unkillable_task(p, memcg, nodemask))
  	return 0;

.

For normal OOM case, out_of_memory() is calling

  oom_scan_process_thread(oc, p, totalpages)
  oom_badness(p, NULL, oc->nodemask, totalpages)

which is translated to

  if (oom_unkillable_task(p, NULL, oc->nodemask))
      return OOM_SCAN_CONTINUE;
  if (oom_unkillable_task(p, NULL, oc->nodemask))
      return 0;

.

But for memcg OOM case, mem_cgroup_out_of_memory() is calling

  oom_scan_process_thread(oc, p, totalpages)
  oom_badness(p, memcg, NULL, totalpages)

which is translated to

  if (oom_unkillable_task(p, NULL, NULL))
      return OOM_SCAN_CONTINUE;
  if (oom_unkillable_task(p, memcg, NULL))
      return 0;

.

Commit 9cbb78bb314360a8 changed oom_scan_process_thread() to
always pass memcg == NULL by removing memcg argument from
oom_scan_process_thread(). As a result, after that commit,
we are doing test_tsk_thread_flag(p, TIF_MEMDIE) check and
oom_task_origin(p) check between two oom_unkillable_task()
calls of memcg OOM case. Why don't we skip these checks by
passing memcg != NULL to first oom_unkillable_task() call?
Was this change by error?

----------
>From 36f79bc270858e93be75de2adae1afe757d91b94 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 18 Feb 2016 19:21:26 +0900
Subject: [PATCH] mm,oom: kill duplicated oom_unkillable_task() checks.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/proc/base.c      |  2 +-
 include/linux/oom.h | 16 +++-------
 mm/memcontrol.c     | 23 ++++----------
 mm/oom_kill.c       | 91 +++++++++++++++++++++++------------------------------
 4 files changed, 51 insertions(+), 81 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index d7c51ca..3020aa2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -581,7 +581,7 @@ static int proc_oom_score(struct seq_file *m, struct pid_namespace *ns,
 
 	read_lock(&tasklist_lock);
 	if (pid_alive(task))
-		points = oom_badness(task, NULL, NULL, totalpages) *
+		points = oom_badness(task, NULL, NULL, totalpages, false) *
 						1000 / totalpages;
 	read_unlock(&tasklist_lock);
 	seq_printf(m, "%lu\n", points);
diff --git a/include/linux/oom.h b/include/linux/oom.h
index 45993b8..b31467e 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -43,13 +43,6 @@ enum oom_constraint {
 	CONSTRAINT_MEMCG,
 };
 
-enum oom_scan_t {
-	OOM_SCAN_OK,		/* scan thread and find its badness */
-	OOM_SCAN_CONTINUE,	/* do not consider thread for oom kill */
-	OOM_SCAN_ABORT,		/* abort the iteration and return */
-	OOM_SCAN_SELECT,	/* always select this thread first */
-};
-
 /* Thread is the potential origin of an oom condition; kill first on oom */
 #define OOM_FLAG_ORIGIN		((__force oom_flags_t)0x1)
 
@@ -73,8 +66,10 @@ static inline bool oom_task_origin(const struct task_struct *p)
 extern void mark_oom_victim(struct task_struct *tsk);
 
 extern unsigned long oom_badness(struct task_struct *p,
-		struct mem_cgroup *memcg, const nodemask_t *nodemask,
-		unsigned long totalpages);
+				 struct mem_cgroup *memcg,
+				 struct oom_control *oc,
+				 unsigned long totalpages,
+				 bool check_exceptions);
 
 extern int oom_kills_count(void);
 extern void note_oom_kill(void);
@@ -86,9 +81,6 @@ extern void check_panic_on_oom(struct oom_control *oc,
 			       enum oom_constraint constraint,
 			       struct mem_cgroup *memcg);
 
-extern enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
-		struct task_struct *task, unsigned long totalpages);
-
 extern bool out_of_memory(struct oom_control *oc);
 
 extern void exit_oom_victim(struct task_struct *tsk);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ae8b81c..0d422bf 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1248,7 +1248,7 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	struct mem_cgroup *iter;
 	unsigned long chosen_points = 0;
 	unsigned long totalpages;
-	unsigned int points = 0;
+	unsigned long points = 0;
 	struct task_struct *chosen = NULL;
 
 	mutex_lock(&oom_lock);
@@ -1271,28 +1271,17 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
 
 		css_task_iter_start(&iter->css, &it);
 		while ((task = css_task_iter_next(&it))) {
-			switch (oom_scan_process_thread(&oc, task, totalpages)) {
-			case OOM_SCAN_SELECT:
-				if (chosen)
-					put_task_struct(chosen);
-				chosen = task;
-				chosen_points = ULONG_MAX;
-				get_task_struct(chosen);
-				/* fall through */
-			case OOM_SCAN_CONTINUE:
+			points = oom_badness(task, memcg, &oc, totalpages,
+					     true);
+			if (!points || points < chosen_points)
 				continue;
-			case OOM_SCAN_ABORT:
+			if (points == ULONG_MAX) {
 				css_task_iter_end(&it);
 				mem_cgroup_iter_break(memcg, iter);
 				if (chosen)
 					put_task_struct(chosen);
 				goto unlock;
-			case OOM_SCAN_OK:
-				break;
-			};
-			points = oom_badness(task, memcg, NULL, totalpages);
-			if (!points || points < chosen_points)
-				continue;
+			}
 			/* Prefer thread group leaders for display purposes */
 			if (points == chosen_points &&
 			    thread_group_leader(chosen))
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index d7bb9c1..f426ce8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -155,21 +155,40 @@ static bool oom_unkillable_task(struct task_struct *p,
 /**
  * oom_badness - heuristic function to determine which candidate task to kill
  * @p: task struct of which task we should calculate
+ * @memcg: memory cgroup. NULL unless memcg OOM case.
+ * @oc: oom_control struct. NULL if /proc/pid/oom_score_adj case.
  * @totalpages: total present RAM allowed for page allocation
+ * @check_exceptions: whether to check for TIF_MEMDIE and oom_task_origin().
+ *
+ * Returns ULONG_MAX if @p is marked as OOM-victim.
+ * Returns ULONG_MAX - 1 if @p is marked as oom_task_origin().
+ * Returns 0 if @p is marked as OOM-unkillable.
+ * Returns integer between 1 and ULONG_MAX - 2 otherwise.
  *
  * The heuristic for determining which task to kill is made to be as simple and
  * predictable as possible.  The goal is to return the highest value for the
  * task consuming the most memory to avoid subsequent oom failures.
  */
 unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
-			  const nodemask_t *nodemask, unsigned long totalpages)
+			  struct oom_control *oc, unsigned long totalpages,
+			  bool check_exceptions)
 {
 	long points;
 	long adj;
 
-	if (oom_unkillable_task(p, memcg, nodemask))
+	if (oom_unkillable_task(p, memcg, oc ? oc->nodemask : NULL))
 		return 0;
 
+	/*
+	 * This task already has access to memory reserves and is being killed.
+	 * Don't allow any other task to have access to the reserves.
+	 */
+	if (check_exceptions)
+		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
+			if (!is_sysrq_oom(oc))
+				return ULONG_MAX;
+		}
+
 	p = find_lock_task_mm(p);
 	if (!p)
 		return 0;
@@ -181,6 +200,16 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
 	}
 
 	/*
+	 * If task is allocating a lot of memory and has been marked to be
+	 * killed first if it triggers an oom, then select it.
+	 */
+	if (check_exceptions)
+		if (oom_task_origin(p)) {
+			task_unlock(p);
+			return ULONG_MAX - 1;
+		}
+
+	/*
 	 * The baseline for the badness score is the proportion of RAM that each
 	 * task's rss, pagetable and swap space use.
 	 */
@@ -268,36 +297,6 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc,
 }
 #endif
 
-enum oom_scan_t oom_scan_process_thread(struct oom_control *oc,
-			struct task_struct *task, unsigned long totalpages)
-{
-	if (oom_unkillable_task(task, NULL, oc->nodemask))
-		return OOM_SCAN_CONTINUE;
-
-	/*
-	 * This task already has access to memory reserves and is being killed.
-	 * Don't allow any other task to have access to the reserves.
-	 */
-	if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
-		if (!is_sysrq_oom(oc))
-			return OOM_SCAN_ABORT;
-	}
-	if (!task->mm)
-		return OOM_SCAN_CONTINUE;
-
-	/*
-	 * If task is allocating a lot of memory and has been marked to be
-	 * killed first if it triggers an oom, then select it.
-	 */
-	if (oom_task_origin(task))
-		return OOM_SCAN_SELECT;
-
-	if (task_will_free_mem(task) && !is_sysrq_oom(oc))
-		return OOM_SCAN_ABORT;
-
-	return OOM_SCAN_OK;
-}
-
 /*
  * Simple selection loop. We chose the process with the highest
  * number of 'points'.  Returns -1 on scan abort.
@@ -311,24 +310,14 @@ static struct task_struct *select_bad_process(struct oom_control *oc,
 
 	rcu_read_lock();
 	for_each_process_thread(g, p) {
-		unsigned int points;
-
-		switch (oom_scan_process_thread(oc, p, totalpages)) {
-		case OOM_SCAN_SELECT:
-			chosen = p;
-			chosen_points = ULONG_MAX;
-			/* fall through */
-		case OOM_SCAN_CONTINUE:
+		const unsigned long points = oom_badness(p, NULL, oc,
+							 totalpages, true);
+		if (!points || points < chosen_points)
 			continue;
-		case OOM_SCAN_ABORT:
+		if (points == ULONG_MAX) {
 			rcu_read_unlock();
 			return (struct task_struct *)(-1UL);
-		case OOM_SCAN_OK:
-			break;
-		};
-		points = oom_badness(p, NULL, oc->nodemask, totalpages);
-		if (!points || points < chosen_points)
-			continue;
+		}
 		/* Prefer thread group leaders for display purposes */
 		if (points == chosen_points && thread_group_leader(chosen))
 			continue;
@@ -679,7 +668,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	struct task_struct *child;
 	struct task_struct *t;
 	struct mm_struct *mm;
-	unsigned int victim_points = 0;
+	unsigned long victim_points = 0;
 	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
 					      DEFAULT_RATELIMIT_BURST);
 	bool can_oom_reap = true;
@@ -712,15 +701,15 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	read_lock(&tasklist_lock);
 	for_each_thread(p, t) {
 		list_for_each_entry(child, &t->children, sibling) {
-			unsigned int child_points;
+			unsigned long child_points;
 
 			if (process_shares_mm(child, p->mm))
 				continue;
 			/*
 			 * oom_badness() returns 0 if the thread is unkillable
 			 */
-			child_points = oom_badness(child, memcg, oc->nodemask,
-								totalpages);
+			child_points = oom_badness(child, memcg, oc,
+						   totalpages, false);
 			if (child_points > victim_points) {
 				put_task_struct(victim);
 				victim = child;
-- 
1.8.3.1

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

#1337303 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Thu 18-02-16 19:30:12, Tetsuo Handa wrote:
[...]
> Commit 9cbb78bb314360a8 changed oom_scan_process_thread() to
> always pass memcg == NULL by removing memcg argument from
> oom_scan_process_thread(). As a result, after that commit,
> we are doing test_tsk_thread_flag(p, TIF_MEMDIE) check and
> oom_task_origin(p) check between two oom_unkillable_task()
> calls of memcg OOM case. Why don't we skip these checks by
> passing memcg != NULL to first oom_unkillable_task() call?
> Was this change by error?

I am not really sure I understand your question.  The point is
that mem_cgroup_out_of_memory does for_each_mem_cgroup_tree which
means that only tasks from the given memcg hierarchy is checked and
oom_unkillable_task cares about memcg only for

        /* When mem_cgroup_out_of_memory() and p is not member of the group */
        if (memcg && !task_in_mem_cgroup(p, memcg))
                return true;

which is never true by definition. I guess we can safely remove the memcg
argument from oom_badness and oom_unkillable_task. At least from a quick
glance...
-- 
Michal Hocko
SUSE Labs

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

#1337306 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Thu 18-02-16 13:08:49, Michal Hocko wrote:
> I guess we can safely remove the memcg
> argument from oom_badness and oom_unkillable_task. At least from a quick
> glance...

No we cannot actually. oom_kill_process could select a child which is in
a different memcg in that case...
-- 
Michal Hocko
SUSE Labs

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

#1338269

Michal Hocko wrote:
> On Thu 18-02-16 13:08:49, Michal Hocko wrote:
> > I guess we can safely remove the memcg
> > argument from oom_badness and oom_unkillable_task. At least from a quick
> > glance...
> 
> No we cannot actually. oom_kill_process could select a child which is in
> a different memcg in that case...

Then, don't we need to check whether processes sharing victim->mm in other
thread groups are in the same memcg when we walk the process list?

----------
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f426ce8..d05db31 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -762,8 +762,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 (oom_badness(p, memcg, oc, totalpages, false) == 0) {
 			/*
 			 * We cannot use oom_reaper for the mm shared by this
 			 * process because it wouldn't get killed and so the
----------

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

#1338285 — Re: [PATCH v2] mm,oom: exclude oom_task_origin processes if they are OOM-unkillable.

On Sat 20-02-16 00:07:05, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 18-02-16 13:08:49, Michal Hocko wrote:
> > > I guess we can safely remove the memcg
> > > argument from oom_badness and oom_unkillable_task. At least from a quick
> > > glance...
> > 
> > No we cannot actually. oom_kill_process could select a child which is in
> > a different memcg in that case...
> 
> Then, don't we need to check whether processes sharing victim->mm in other
> thread groups are in the same memcg when we walk the process list?

memcg is bound to the mm not to the task. So all processes sharing the
mm after in the same memcg (from the memcg POV). See tast_struct::owner.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web