Skip to contentSkip to conversation

3rd pile of OOM patch series

PATCH In review KOSAKI Motohiro 4 participants 25 messages Jun 8, 2010 – Jun 16, 2010

Subsystems (8)
lore
Share & tools
lore
#0 lore raw

cover 3rd pile of OOM patch series

This is 3rd pile of collection of OOM patches.
I think they don't immix anyone objected patch. but please double check.

=====================================================================

 Documentation/sysctl/vm.txt |    2 +-
 fs/proc/base.c              |    4 +-
 include/linux/mempolicy.h   |   13 +++-
 include/linux/oom.h         |    8 ++
 kernel/sysctl.c             |    4 +-
 mm/mempolicy.c              |   44 ++++++++++++
 mm/oom_kill.c               |  156 ++++++++++++++++++++++---------------------
 7 files changed, 146 insertions(+), 85 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#1 lore raw

PATCH 1/10 oom: don't try to kill oom_unkillable child

Now, badness() doesn't care neigher CPUSET nor mempolicy. Then if the victim child process is oom_unkillable()==1, __out_of_memory() can makes kernel hang eventually.

This patch fixes it.

[remark: this is needed to fold "oom: sacrifice child with highest badness score for parent"]

---
 mm/oom_kill.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
mm/oom_kill.c +2 -3
index d49d542..0d7397b 100644
@@ -387,9 +387,6 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
 static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem,
 			      int verbose)
 {
-	if (oom_unkillable(p, mem))
-		return 1;
-
 	p = find_lock_task_mm(p);
 	if (!p)
 		return 1;
@@ -440,6 +437,8 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 
 			if (c->mm == p->mm)
 				continue;
+			if (oom_unkillable(c, mem, nodemask))
+				continue;
 
 			/* badness() returns 0 if the thread is unkillable */
 			cpoints = badness(c, uptime.tv_sec);
Signature · 1 line
1.6.5.2
#2 lore raw

Re: [PATCH 01/10] oom: don't try to kill oom_unkillable child

Quoted 29 lines · KOSAKI Motohiro

Now, badness() doesn't care neigher CPUSET nor mempolicy. Then if the victim child process is oom_unkillable()==1, __out_of_memory() can makes kernel hang eventually.

This patch fixes it.

[remark: this is needed to fold "oom: sacrifice child with highest badness score for parent"]

---
 mm/oom_kill.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
mm/oom_kill.c +0 -3
index d49d542..0d7397b 100644
@@ -387,9 +387,6 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
 static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem,
 			      int verbose)
 {
-	if (oom_unkillable(p, mem))
-		return 1;
-
 	p = find_lock_task_mm(p);
 	if (!p)
 		return 1;

mm/oom_kill.c static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order, · line 444

Quoted 8 lines
mm/oom_kill.c +2 -0
@@ -440,6 +437,8 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 
 			if (c->mm == p->mm)
 				continue;
+			if (oom_unkillable(c, mem, nodemask))
+				continue;
 
 			/* badness() returns 0 if the thread is unkillable */
 			cpoints = badness(c, uptime.tv_sec);

This doesn't apply to anything.

#3 lore raw

PATCH 2/10 oom: remove verbose argument from __oom_kill_process()

In reply to KOSAKI Motohiro

Now, verbose argument is unused. This patch remove it.

[remark: this patch is needed to fold to "oom: remove PF_EXITING check completely"]

---
 mm/oom_kill.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)
mm/oom_kill.c +9 -11
index 0d7397b..3c83fba 100644
@@ -384,20 +384,18 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
  * flag though it's unlikely that  we select a process with CAP_SYS_RAW_IO
  * set.
  */
-static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem,
-			      int verbose)
+static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem)
 {
 	p = find_lock_task_mm(p);
 	if (!p)
 		return 1;
 
-	if (verbose)
-		printk(KERN_ERR "Killed process %d (%s) "
-		       "vsz:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
-		       task_pid_nr(p), p->comm,
-		       K(p->mm->total_vm),
-		       K(get_mm_counter(p->mm, MM_ANONPAGES)),
-		       K(get_mm_counter(p->mm, MM_FILEPAGES)));
+	printk(KERN_ERR "Killed process %d (%s) "
+	       "vsz:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
+	       task_pid_nr(p), p->comm,
+	       K(p->mm->total_vm),
+	       K(get_mm_counter(p->mm, MM_ANONPAGES)),
+	       K(get_mm_counter(p->mm, MM_FILEPAGES)));
 	task_unlock(p);
 
 	/*
@@ -437,7 +435,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 
 			if (c->mm == p->mm)
 				continue;
-			if (oom_unkillable(c, mem, nodemask))
+			if (oom_unkillable(c, mem))
 				continue;
 
 			/* badness() returns 0 if the thread is unkillable */
@@ -449,7 +447,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 		}
 	} while_each_thread(p, t);
 
-	return __oom_kill_process(victim, mem, 1);
+	return __oom_kill_process(victim, mem);
 }
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#4 lore raw

Re: [PATCH 02/10] oom: remove verbose argument from __oom_kill_process()

Quoted 2 lines · KOSAKI Motohiro

Now, verbose argument is unused. This patch remove it.

This is already done in my oom killer rewrite, please work with other developers in developing their work instead of acting as a maintainer, which you aren't.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#5 lore raw

PATCH 3/10 oom: rename badness() to oom_badness()

In reply to KOSAKI Motohiro

badness() is wrong name because it's too generic name.

rename it.

---
 fs/proc/base.c      |    4 +---
 include/linux/oom.h |    2 ++
 mm/oom_kill.c       |   10 +++++-----
 3 files changed, 8 insertions(+), 8 deletions(-)
fs/proc/base.c +1 -3
index 2222102..c099f03 100644
@@ -427,8 +427,6 @@ static const struct file_operations proc_lstats_operations = {
 
 #endif
 
-/* The badness from the OOM killer */
-unsigned long badness(struct task_struct *p, unsigned long uptime);
 static int proc_oom_score(struct task_struct *task, char *buffer)
 {
 	unsigned long points = 0;
@@ -437,7 +435,7 @@ static int proc_oom_score(struct task_struct *task, char *buffer)
 	do_posix_clock_monotonic_gettime(&uptime);
 	read_lock(&tasklist_lock);
 	if (pid_alive(task))
-		points = badness(task, uptime.tv_sec);
+		points = oom_badness(task, uptime.tv_sec);
 	read_unlock(&tasklist_lock);
 	return sprintf(buffer, "%lu\n", points);
 }
include/linux/oom.h +2 -0
index 5376623..effb223 100644
@@ -14,6 +14,7 @@
 
 struct zonelist;
 struct notifier_block;
+struct task_struct;
 
 /*
  * Types of limitations to the nodes from which allocations may occur
@@ -29,6 +30,7 @@ extern void clear_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
 
 extern void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
 		int order, nodemask_t *mask);
+extern unsigned long oom_badness(struct task_struct *p, unsigned long uptime);
 extern int register_oom_notifier(struct notifier_block *nb);
 extern int unregister_oom_notifier(struct notifier_block *nb);
 
mm/oom_kill.c +5 -5
index 3c83fba..80492ff 100644
@@ -66,7 +66,7 @@ static struct task_struct *find_lock_task_mm(struct task_struct *p)
 }
 
 /**
- * badness - calculate a numeric value for how bad this task has been
+ * oom_badness - calculate a numeric value for how bad this task has been
  * @p: task struct of which task we should calculate
  * @uptime: current uptime in seconds
  *
@@ -84,7 +84,7 @@ static struct task_struct *find_lock_task_mm(struct task_struct *p)
  *    of least surprise ... (be careful when you change it)
  */
 
-unsigned long badness(struct task_struct *p, unsigned long uptime)
+unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
 {
 	unsigned long points, cpu_time, run_time;
 	struct task_struct *c;
@@ -302,7 +302,7 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
 		if (test_tsk_thread_flag(p, TIF_MEMDIE))
 			return ERR_PTR(-1UL);
 
-		points = badness(p, uptime.tv_sec);
+		points = oom_badness(p, uptime.tv_sec);
 		if (points > *ppoints || !chosen) {
 			chosen = p;
 			*ppoints = points;
@@ -438,8 +438,8 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			if (oom_unkillable(c, mem))
 				continue;
 
-			/* badness() returns 0 if the thread is unkillable */
-			cpoints = badness(c, uptime.tv_sec);
+			/* oom_badness() returns 0 if the thread is unkillable */
+			cpoints = oom_badness(c, uptime.tv_sec);
 			if (cpoints > victim_points) {
 				victim = c;
 				victim_points = cpoints;
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#6 lore raw

Re: [PATCH 03/10] oom: rename badness() to oom_badness()

Quoted 4 lines · KOSAKI Motohiro

badness() is wrong name because it's too generic name.

rename it.

This is already renamed in my heuristic rewrite which we all agree, in principle, is needed. I'm really confused that you're trying to take little bits and pieces of my work like this, using my name for the function in this case, for example, and not working with other developers in reviewing their work.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#7 lore raw

PATCH 4/10 oom: move sysctl declarations to oom.h

In reply to KOSAKI Motohiro

From: David Rientjes <[email protected]>

The three oom killer sysctl variables (sysctl_oom_dump_tasks, sysctl_oom_kill_allocating_task, and sysctl_panic_on_oom) are better declared in include/linux/oom.h rather than kernel/sysctl.c.

---
 include/linux/oom.h |    6 ++++++
 kernel/sysctl.c     |    4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)
include/linux/oom.h +6 -0
index effb223..14eb449 100644
@@ -45,5 +45,11 @@ static inline void oom_killer_enable(void)
 {
 	oom_killer_disabled = false;
 }
+
+/* sysctls */
+extern int sysctl_oom_dump_tasks;
+extern int sysctl_oom_kill_allocating_task;
+extern int sysctl_panic_on_oom;
+
 #endif /* __KERNEL__*/
 #endif /* _INCLUDE_LINUX_OOM_H */
kernel/sysctl.c +1 -3
index 88ac884..4dfd618 100644
@@ -55,6 +55,7 @@
 #include <linux/perf_event.h>
 #include <linux/kprobes.h>
 #include <linux/pipe_fs_i.h>
+#include <linux/oom.h>
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
@@ -87,9 +88,6 @@
 /* External variables not in a header file. */
 extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
-extern int sysctl_panic_on_oom;
-extern int sysctl_oom_kill_allocating_task;
-extern int sysctl_oom_dump_tasks;
 extern int max_threads;
 extern int core_uses_pid;
 extern int suid_dumpable;
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#8 lore raw

PATCH 5/10 oom: enable oom tasklist dump by default

In reply to KOSAKI Motohiro

From: David Rientjes <[email protected]>

The oom killer tasklist dump, enabled with the oom_dump_tasks sysctl, is very helpful information in diagnosing why a user's task has been killed. It emits useful information such as each eligible thread's memory usage that can determine why the system is oom, so it should be enabled by default.

---
 Documentation/sysctl/vm.txt |    2 +-
 mm/oom_kill.c               |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Documentation/sysctl/vm.txt +1 -1
index 5fdbb61..3936b61 100644
@@ -511,7 +511,7 @@ information may not be desired.
 If this is set to non-zero, this information is shown whenever the
 OOM killer actually kills a memory-hogging task.
 
-The default value is 0.
+The default value is 1 (enabled).
 
 ==============================================================
 
mm/oom_kill.c +1 -1
index 80492ff..b88172c 100644
@@ -31,7 +31,7 @@
 
 int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;
-int sysctl_oom_dump_tasks;
+int sysctl_oom_dump_tasks = 1;
 static DEFINE_SPINLOCK(zone_scan_lock);
 /* #define DEBUG */
 
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#9 lore raw

PATCH 6/10 oom: cleanup has_intersects_mems_allowed()

In reply to KOSAKI Motohiro

Now has_intersects_mems_allowed() has own thread iterate logic, but it should use while_each_thread().

It slightly improve the code readability.

---
 mm/oom_kill.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)
mm/oom_kill.c +3 -5
index b88172c..8376ad1 100644
@@ -38,16 +38,14 @@ static DEFINE_SPINLOCK(zone_scan_lock);
 /*
  * Is all threads of the target process nodes overlap ours?
  */
-static int has_intersects_mems_allowed(struct task_struct *tsk)
+static int has_intersects_mems_allowed(struct task_struct *p)
 {
-	struct task_struct *t;
+	struct task_struct *t = p;
 
-	t = tsk;
 	do {
 		if (cpuset_mems_allowed_intersects(current, t))
 			return 1;
-		t = next_thread(t);
-	} while (t != tsk);
+	} while_each_thread(p, t);
 
 	return 0;
 }
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#10 lore raw

Re: [PATCH 06/10] oom: cleanup has_intersects_mems_allowed()

Quoted 5 lines · KOSAKI Motohiro

Now has_intersects_mems_allowed() has own thread iterate logic, but it should use while_each_thread().

It slightly improve the code readability.

These cleanups should be done on top of my oom killer rewrite instead, please work with others in their work instead of getting in the way of it time and time again.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#11 lore raw

PATCH 7/10 oom: kill useless debug print

In reply to KOSAKI Motohiro

Now, all of oom developers usually are using sysctl_oom_dump_tasks. Redundunt useless debug print can be removed.

---
 mm/oom_kill.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)
mm/oom_kill.c +0 -5
index 8376ad1..e7d3a5d 100644
@@ -33,7 +33,6 @@ int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;
 int sysctl_oom_dump_tasks = 1;
 static DEFINE_SPINLOCK(zone_scan_lock);
-/* #define DEBUG */
 
 /*
  * Is all threads of the target process nodes overlap ours?
@@ -201,10 +200,6 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
 			points >>= -(oom_adj);
 	}
 
-#ifdef DEBUG
-	printk(KERN_DEBUG "OOMkill: task %d (%s) got %lu points\n",
-	p->pid, p->comm, points);
-#endif
 	return points;
 }
 
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#12 lore raw

Re: [PATCH 07/10] oom: kill useless debug print

Quoted 3 lines · KOSAKI Motohiro

Now, all of oom developers usually are using sysctl_oom_dump_tasks. Redundunt useless debug print can be removed.

This is already removed with my heuristic rewrite as you well know, why are you constantly trying to get in the way of that work?

#13 lore raw

PATCH 8/10 oom: use send_sig() instead force_sig()

In reply to KOSAKI Motohiro

Oleg, I parsed your mention mean following patch, correct?

===========================================
Oleg pointed out oom_kill.c has force_sig() abuse. force_sig() mean ignore signal mask. but SIGKILL itself is not maskable. So, we can use send_sig() sefely.

---
 mm/oom_kill.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/oom_kill.c +1 -1
index e7d3a5d..599f977 100644
@@ -399,7 +399,7 @@ static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem)
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 
-	force_sig(SIGKILL, p);
+	send_sig(SIGKILL, p, 1);
 
 	return 0;
 }
Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#14 lore raw

Re: [PATCH 08/10] oom: use send_sig() instead force_sig()

Quoted 3 lines · KOSAKI Motohiro

Oleg pointed out oom_kill.c has force_sig() abuse. force_sig() mean ignore signal mask. but SIGKILL itself is not maskable.

Yes. And we have other reasons to avoid force_sig(). It should be used only for synchronous signals.

But,

mm/oom_kill.c static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem) · line 402

Quoted 6 lines
mm/oom_kill.c +1 -1
@@ -399,7 +399,7 @@ static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem)
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 
-	force_sig(SIGKILL, p);
+	send_sig(SIGKILL, p, 1);

This is not right, we need send_sig(SIGKILL, p, 0). Better yet, send_sig_info(SIGKILL, SEND_SIG_NOINFO). I think send_sig() should die.

The reason is that si_fromuser() must be true, otherwise we can't kill the SIGNAL_UNKILLABLE (sub-namespace inits) tasks.

Oh. This reminds me, we really need the trivial (but annoying) cleanups here. The usage of SEND_SIG_ constants is messy, and they should be renamed at least.

And in fact, we need the new one which acts like SEND_SIG_FORCED but si_fromuser(). We do not want to allocate the memory when the caller is oom_kill or zap_pid_ns_processes().

OK. I'll send the simple patch which adds the new helper with the comment. send_sigkill() or kernel_kill_task(), or do you see a better name?

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#15 lore raw

cover signals: introduce send_sigkill() helper

Quoted 15 lines · Oleg Nesterov

mm/oom_kill.c static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem) · line 402

Quoted 7 lines
mm/oom_kill.c +1 -1
@@ -399,7 +399,7 @@ static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem)
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 
-	force_sig(SIGKILL, p);
+	send_sig(SIGKILL, p, 1);

This is not right, we need send_sig(SIGKILL, p, 0). Better yet, send_sig_info(SIGKILL, SEND_SIG_NOINFO). I think send_sig() should die.

The reason is that si_fromuser() must be true, otherwise we can't kill the SIGNAL_UNKILLABLE (sub-namespace inits) tasks.

Quoted 7 lines

Oh. This reminds me, we really need the trivial (but annoying) cleanups here. The usage of SEND_SIG_ constants is messy, and they should be renamed at least.

And in fact, we need the new one which acts like SEND_SIG_FORCED but si_fromuser(). We do not want to allocate the memory when the caller is oom_kill or zap_pid_ns_processes().

I tried to make some simple cleanups right now, but this really needs time and discussion.

So. If we are going to remove force_sig() in mm/oom_kill.c (and I think we should), I'd like to add the trivial helper first.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#16 lore raw

PATCH 1/1 signals: introduce send_sigkill() helper

Cleanup, no functional changes.

There are a lot of buggy SIGKILL users in kernel. For example, almost every force_sig(SIGKILL) is wrong. force_sig() is not safe, it assumes that the task has the valid ->sighand, and in general it should be used only for synchronous signals. send_sig(SIGKILL, p, 1) or send_xxx(SEND_SIG_FORCED/SEND_SIG_PRIV) is not right too but this is not immediately obvious.

The only way to correctly send SIGKILL is send_sig_info(SEND_SIG_NOINFO) but we do not want to use this directly, because we can optimize this case later. For example, zap_pid_ns_processes() allocates sigqueue for each process in namespace, this is unneeded.

Introduce the trivial send_sigkill() helper on top of send_sig_info() and change zap_pid_ns_processes() as an example.

Note: we need more cleanups here, this is only the first change.

---

 include/linux/sched.h  |    9 +++++++++
 kernel/pid_namespace.c |    8 +-------
 2 files changed, 10 insertions(+), 7 deletions(-)
include/linux/sched.h +9 -0
@@ -2053,6 +2053,15 @@ static inline int kill_cad_pid(int sig, 
 #define SEND_SIG_PRIV	((struct siginfo *) 1)
 #define SEND_SIG_FORCED	((struct siginfo *) 2)
 
+static inline int send_sigkill(struct task_struct * p)
+{
+	/*
+	 * Kills any user-space task, even SIGNAL_UNKILLABLE.
+	 * We use SEND_SIG_NOINFO to make si_fromuser() true.
+	 */
+	return send_sig_info(SIGKILL, SEND_SIG_NOINFO, p);
+}
+
 /*
  * True if we are on the alternate signal stack.
  */
kernel/pid_namespace.c +1 -7
@@ -160,15 +160,9 @@ void zap_pid_ns_processes(struct pid_nam
 	nr = next_pidmap(pid_ns, 1);
 	while (nr > 0) {
 		rcu_read_lock();
-
-		/*
-		 * Any nested-container's init processes won't ignore the
-		 * SEND_SIG_NOINFO signal, see send_signal()->si_fromuser().
-		 */
 		task = pid_task(find_vpid(nr), PIDTYPE_PID);
 		if (task)
-			send_sig_info(SIGKILL, SEND_SIG_NOINFO, task);
-
+			send_sigkill(task);
 		rcu_read_unlock();
 
 		nr = next_pidmap(pid_ns, nr);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Reviewed-by on v1 1/1
#17 lore raw

Re: [PATCH 1/1] signals: introduce send_sigkill() helper

Quoted 14 lines · On Thu, 10 Jun 2010 03:00:23 +0200 Oleg Nesterov <[email protected]> wrote:

Cleanup, no functional changes.

There are a lot of buggy SIGKILL users in kernel. For example, almost every force_sig(SIGKILL) is wrong. force_sig() is not safe, it assumes that the task has the valid ->sighand, and in general it should be used only for synchronous signals. send_sig(SIGKILL, p, 1) or send_xxx(SEND_SIG_FORCED/SEND_SIG_PRIV) is not right too but this is not immediately obvious.

The only way to correctly send SIGKILL is send_sig_info(SEND_SIG_NOINFO) but we do not want to use this directly, because we can optimize this case later. For example, zap_pid_ns_processes() allocates sigqueue for each process in namespace, this is unneeded.

Quoted 6 lines

Introduce the trivial send_sigkill() helper on top of send_sig_info() and change zap_pid_ns_processes() as an example.

Note: we need more cleanups here, this is only the first change.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author Reviewed-by on v1 1/1
#18 lore raw

Re: [PATCH 1/1] signals: introduce send_sigkill() helper

In reply to Oleg Nesterov
Quoted 14 lines

Cleanup, no functional changes.

There are a lot of buggy SIGKILL users in kernel. For example, almost every force_sig(SIGKILL) is wrong. force_sig() is not safe, it assumes that the task has the valid ->sighand, and in general it should be used only for synchronous signals. send_sig(SIGKILL, p, 1) or send_xxx(SEND_SIG_FORCED/SEND_SIG_PRIV) is not right too but this is not immediately obvious.

The only way to correctly send SIGKILL is send_sig_info(SEND_SIG_NOINFO) but we do not want to use this directly, because we can optimize this case later. For example, zap_pid_ns_processes() allocates sigqueue for each process in namespace, this is unneeded.

Quoted 6 lines

Introduce the trivial send_sigkill() helper on top of send_sig_info() and change zap_pid_ns_processes() as an example.

Note: we need more cleanups here, this is only the first change.

Great.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#19 lore raw

Re: [PATCH 1/1] signals: introduce send_sigkill() helper

In reply to Oleg Nesterov

Andrew, please drop

signals-introduce-send_sigkill-helper.patch

I am stupid.

Quoted 11 lines · Oleg Nesterov

Cleanup, no functional changes.

There are a lot of buggy SIGKILL users in kernel. For example, almost every force_sig(SIGKILL) is wrong. force_sig() is not safe, it assumes that the task has the valid ->sighand, and in general it should be used only for synchronous signals. send_sig(SIGKILL, p, 1) or send_xxx(SEND_SIG_FORCED/SEND_SIG_PRIV) is not right too but this is not immediately obvious.

The only way to correctly send SIGKILL is send_sig_info(SEND_SIG_NOINFO)

No, SEND_SIG_NOINFO doesn't work too. Oh, can't understand what I was thinking about. current is the random task, but send_signal() checks if the caller is from-parent-ns.

Quoted 1 line

Note: we need more cleanups here, this is only the first change.

We need the cleanups first. Until then oom-killer has to use force_sig() if we want to kill the SIGNAL_UNKILLABLE tasks too.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#20 lore raw

Re: [PATCH 1/1] signals: introduce send_sigkill() helper

Quoted 19 lines

Andrew, please drop

signals-introduce-send_sigkill-helper.patch

I am stupid.

Quoted 11 lines · Oleg Nesterov

Cleanup, no functional changes.

There are a lot of buggy SIGKILL users in kernel. For example, almost every force_sig(SIGKILL) is wrong. force_sig() is not safe, it assumes that the task has the valid ->sighand, and in general it should be used only for synchronous signals. send_sig(SIGKILL, p, 1) or send_xxx(SEND_SIG_FORCED/SEND_SIG_PRIV) is not right too but this is not immediately obvious.

The only way to correctly send SIGKILL is send_sig_info(SEND_SIG_NOINFO)

Quoted 8 lines

No, SEND_SIG_NOINFO doesn't work too. Oh, can't understand what I was thinking about. current is the random task, but send_signal() checks if the caller is from-parent-ns.

Quoted 1 line

Note: we need more cleanups here, this is only the first change.

We need the cleanups first. Until then oom-killer has to use force_sig() if we want to kill the SIGNAL_UNKILLABLE tasks too.

This definitely needed. OOM-Killer is not racist ;)

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#21 lore raw

Re: [PATCH 08/10] oom: use send_sig() instead force_sig()

In reply to Oleg Nesterov
Quoted 21 lines · KOSAKI Motohiro
Quoted 3 lines

Oleg pointed out oom_kill.c has force_sig() abuse. force_sig() mean ignore signal mask. but SIGKILL itself is not maskable.

Yes. And we have other reasons to avoid force_sig(). It should be used only for synchronous signals.

But,

mm/oom_kill.c static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem) · line 402

Quoted 6 lines
mm/oom_kill.c +1 -1
@@ -399,7 +399,7 @@ static int __oom_kill_process(struct task_struct *p, struct mem_cgroup *mem)
 	p->rt.time_slice = HZ;
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 
-	force_sig(SIGKILL, p);
+	send_sig(SIGKILL, p, 1);

This is not right, we need send_sig(SIGKILL, p, 0). Better yet, send_sig_info(SIGKILL, SEND_SIG_NOINFO). I think send_sig() should die.

Quoted 2 lines

The reason is that si_fromuser() must be true, otherwise we can't kill the SIGNAL_UNKILLABLE (sub-namespace inits) tasks.

Thanks. I am not signal expert.
To be honest, current special siginfo arguments have a bit unclear meanings to me ;)
current definition (following) doesn't teach anything.

sched.h
=====================
/* These can be the second arg to send_sig_info/send_group_sig_info. */ #define SEND_SIG_NOINFO ((struct siginfo *) 0)
#define SEND_SIG_PRIV ((struct siginfo *) 1)
#define SEND_SIG_FORCED ((struct siginfo *) 2)

If anyone write exact meanings, I'm really really glad.

Quoted 11 lines

Oh. This reminds me, we really need the trivial (but annoying) cleanups here. The usage of SEND_SIG_ constants is messy, and they should be renamed at least.

And in fact, we need the new one which acts like SEND_SIG_FORCED but si_fromuser(). We do not want to allocate the memory when the caller is oom_kill or zap_pid_ns_processes().

OK. I'll send the simple patch which adds the new helper with the comment. send_sigkill() or kernel_kill_task(), or do you see a better name?

Very thanks. both name are pretty good to me.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#22 lore raw

PATCH 9/10 oom: filter tasks not sharing the same cpuset

In reply to KOSAKI Motohiro

From: David Rientjes <[email protected]>

Tasks that do not share the same set of allowed nodes with the task that triggered the oom should not be considered as candidates for oom kill.

Tasks in other cpusets with a disjoint set of mems would be unfairly penalized otherwise because of oom conditions elsewhere; an extreme example could unfairly kill all other applications on the system if a single task in a user's cpuset sets itself to OOM_DISABLE and then uses more memory than allowed.

Killing tasks outside of current's cpuset rarely would free memory for current anyway. To use a sane heuristic, we must ensure that killing a task would likely free memory for current and avoid needlessly killing others at all costs just because their potential memory freeing is unknown. It is better to kill current than another task needlessly.

kosaki: a historical interlude...

We applied the exactly same patch in 2005:

: commit ef08e3b4981aebf2ba9bd7025ef7210e8eec07ce
: Author: Paul Jackson <[email protected]>
: Date: Tue Sep 6 15:18:13 2005 -0700
:
: [PATCH] cpusets: confine oom_killer to mem_exclusive cpuset
:
: Now the real motivation for this cpuset mem_exclusive patch series seems : trivial.
:
: This patch keeps a task in or under one mem_exclusive cpuset from provoking an : oom kill of a task under a non-overlapping mem_exclusive cpuset. Since only : interrupt and GFP_ATOMIC allocations are allowed to escape mem_exclusive : containment, there is little to gain from oom killing a task under a : non-overlapping mem_exclusive cpuset, as almost all kernel and user memory : allocation must come from disjoint memory nodes.
:
: This patch enables configuring a system so that a runaway job under one : mem_exclusive cpuset cannot cause the killing of a job in another such cpuset : that might be using very high compute and memory resources for a prolonged : time.

And we changed it to current logic in 2006

: commit 7887a3da753e1ba8244556cc9a2b38c815bfe256
: Author: Nick Piggin <[email protected]>
: Date: Mon Sep 25 23:31:29 2006 -0700
:
: [PATCH] oom: cpuset hint
:
: cpuset_excl_nodes_overlap does not always indicate that killing a task will : not free any memory we for us. For example, we may be asking for an : allocation from _anywhere_ in the machine, or the task in question may be : pinning memory that is outside its cpuset. Fix this by just causing : cpuset_excl_nodes_overlap to reduce the badness rather than disallow it.

And we haven't get the explanation why this patch doesn't reintroduced an old issue. but I don't refuse a patch if it have multiple ack.

care of oom_kill_allocating_task case and dump_tasks]

---
 mm/oom_kill.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)
mm/oom_kill.c +7 -9
index 599f977..f45ac18 100644
@@ -35,7 +35,7 @@ int sysctl_oom_dump_tasks = 1;
 static DEFINE_SPINLOCK(zone_scan_lock);
 
 /*
- * Is all threads of the target process nodes overlap ours?
+ * Do all threads of the target process overlap our allowed nodes?
  */
 static int has_intersects_mems_allowed(struct task_struct *p)
 {
@@ -181,14 +181,6 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
 		points /= 4;
 
 	/*
-	 * If p's nodes don't overlap ours, it may still help to kill p
-	 * because p may have allocated or otherwise mapped memory on
-	 * this node before. However it will be less likely.
-	 */
-	if (!has_intersects_mems_allowed(p))
-		points /= 8;
-
-	/*
 	 * Adjust the score by oom_adj.
 	 */
 	if (oom_adj) {
@@ -259,6 +251,10 @@ static int oom_unkillable(struct task_struct *p, struct mem_cgroup *mem)
 	if (p->signal->oom_adj == OOM_DISABLE)
 		return 1;
 
+	/* If p's nodes don't overlap ours, it may not help to kill p. */
+	if (!has_intersects_mems_allowed(p))
+		return 1;
+
 	return 0;
 }
 
@@ -336,6 +332,8 @@ static void dump_tasks(const struct mem_cgroup *mem)
 			continue;
 		if (mem && !task_in_mem_cgroup(p, mem))
 			continue;
+		if (!has_intersects_mems_allowed(p))
+			continue;
 
 		task = find_lock_task_mm(p);
 		if (!task)
Signature · 1 line
1.6.5.2
#23 lore raw

Re: [PATCH 09/10] oom: filter tasks not sharing the same cpuset

Quoted 5 lines · KOSAKI Motohiro

From: David Rientjes <[email protected]>

Tasks that do not share the same set of allowed nodes with the task that triggered the oom should not be considered as candidates for oom kill.

You're not a maintainer, as I obviously have to point out to you often enough. I've repeatedly asked you to work with me in reviewing my oom killer rewrite on linux-mm, yet you seldom offer any valuable feedback other than a simple "nack". I don't consider any of your patchset here to be more applicable than my patchset, which has been developed over the course of several months, and your lack of participation in the process is really quite shocking to me.

In case nobody has told you before: Andrew maintains this code and these patches will be going through the -mm tree. You are not a maintainer of it (or any other kernel code), so please act within your role of kernel hacker and review patches as people propose them by offering your constructive feedback.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#24 lore raw

PATCH 10/10 oom: select task from tasklist for mempolicy ooms

In reply to KOSAKI Motohiro

From: David Rientjes <[email protected]>

The oom killer presently kills current whenever there is no more memory free or reclaimable on its mempolicy's nodes. There is no guarantee that current is a memory-hogging task or that killing it will free any substantial amount of memory, however.

In such situations, it is better to scan the tasklist for nodes that are allowed to allocate on current's set of nodes and kill the task with the highest badness() score. This ensures that the most memory-hogging task, or the one configured by the user with /proc/pid/oom_adj, is always selected in such scenarios.

---
 include/linux/mempolicy.h |   13 +++++-
 mm/mempolicy.c            |   44 ++++++++++++++++++++
 mm/oom_kill.c             |   98 +++++++++++++++++++++++++-------------------
 3 files changed, 112 insertions(+), 43 deletions(-)
include/linux/mempolicy.h +12 -1
index 7b9ef6b..9c84270 100644
@@ -210,6 +210,8 @@ extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
 				unsigned long addr, gfp_t gfp_flags,
 				struct mempolicy **mpol, nodemask_t **nodemask);
 extern bool init_nodemask_of_mempolicy(nodemask_t *mask);
+extern bool mempolicy_nodemask_intersects(struct task_struct *tsk,
+					  const nodemask_t *mask);
 extern unsigned slab_node(struct mempolicy *policy);
 
 extern enum zone_type policy_zone;
@@ -338,7 +340,16 @@ static inline struct zonelist *huge_zonelist(struct vm_area_struct *vma,
 	return node_zonelist(0, gfp_flags);
 }
 
-static inline bool init_nodemask_of_mempolicy(nodemask_t *m) { return false; }
+static inline bool init_nodemask_of_mempolicy(nodemask_t *m)
+{
+	return false;
+}
+
+static inline bool mempolicy_nodemask_intersects(struct task_struct *tsk,
+						 const nodemask_t *mask)
+{
+	return false;
+}
 
 static inline int do_migrate_pages(struct mm_struct *mm,
 			const nodemask_t *from_nodes,
mm/mempolicy.c +44 -0

50 lines not on this page · raw message

mm/oom_kill.c +56 -42

245 lines not on this page · raw message

Signature · 9 lines
1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>