Now, badness() doesn't care neigher CPUSET nor mempolicy. Then
if the victim child process have disjoint nodemask, __out_of_memory()
can makes kernel hang eventually.
This patch fixes it.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 16 +++++++++-------
1 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 26ae697..0623c3d 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -429,7 +429,7 @@ static int oom_kill_task(struct task_struct *p)
static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
unsigned long points, struct mem_cgroup *mem,
- const char *message)
+ nodemask_t *nodemask, const char *message)
{
struct task_struct *victim = p;
struct task_struct *child;
@@ -469,6 +469,8 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
continue;
if (mem && !task_in_mem_cgroup(child, mem))
continue;
+ if (!has_intersects_mems_allowed(child, nodemask))
+ continue;
/* badness() returns 0 if the thread is unkillable */
child_points = badness(child, uptime.tv_sec);
@@ -519,7 +521,7 @@ retry:
if (!p || PTR_ERR(p) == -1UL)
goto out;
- if (oom_kill_process(p, gfp_mask, 0, points, mem,
+ if (oom_kill_process(p, gfp_mask, 0, points, mem, NULL,
"Memory cgroup out of memory"))
goto retry;
out:
@@ -669,6 +671,8 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
*/
if (zonelist)
constraint = constrained_alloc(zonelist, gfp_mask, nodemask);
+ if (constraint != CONSTRAINT_MEMORY_POLICY)
+ nodemask = NULL;
check_panic_on_oom(constraint, gfp_mask, order);
read_lock(&tasklist_lock);
@@ -678,15 +682,13 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
* non-zero, current could not be killed so we must fallback to
* the tasklist scan.
*/
- if (!oom_kill_process(current, gfp_mask, order, 0, NULL,
+ if (!oom_kill_process(current, gfp_mask, order, 0, NULL, nodemask,
"Out of memory (oom_kill_allocating_task)"))
return;
}
retry:
- p = select_bad_process(&points, NULL,
- constraint == CONSTRAINT_MEMORY_POLICY ? nodemask :
- NULL);
+ p = select_bad_process(&points, NULL, nodemask);
if (PTR_ERR(p) == -1UL)
return;
@@ -697,7 +699,7 @@ retry:
panic("Out of memory and no killable processes...\n");
}
- if (oom_kill_process(p, gfp_mask, order, points, NULL,
+ if (oom_kill_process(p, gfp_mask, order, points, NULL, nodemask,
"Out of memory"))
goto retry;
read_unlock(&tasklist_lock);
--
1.6.5.2
badness() is wrong name because it's too generic name. rename it.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
fs/proc/base.c | 4 +---
include/linux/oom.h | 3 +++
mm/oom_kill.c | 13 ++++++-------
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 28099a1..d33c43c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -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);
}
diff --git a/include/linux/oom.h b/include/linux/oom.h
index 9d7c34a..4cd6b89 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -14,6 +14,7 @@
struct zonelist;
struct notifier_block;
+struct task_struct;
/*
* Types of limitations to the nodes from which allocations may occur
@@ -25,6 +26,8 @@ enum oom_constraint {
CONSTRAINT_MEMCG,
};
+
+extern unsigned long oom_badness(struct task_struct *p, unsigned long uptime);
extern int try_set_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
extern void clear_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0623c3d..1969cc1 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -102,7 +102,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
*
@@ -119,8 +119,7 @@ static struct task_struct *find_lock_task_mm(struct task_struct *p)
* algorithm has been meticulously tuned to meet the principle
* 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 *child;
@@ -336,7 +335,7 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
if (p->signal->oom_adj == OOM_DISABLE)
continue;
- points = badness(p, uptime.tv_sec);
+ points = oom_badness(p, uptime.tv_sec);
if (points > *ppoints || !chosen) {
chosen = p;
*ppoints = points;
@@ -456,7 +455,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
/*
* If any of p's children has a different mm and is eligible for kill,
- * the one with the highest badness() score is sacrificed for its
+ * the one with the highest oom_badness() score is sacrificed for its
* parent. This attempts to lose the minimal amount of work done while
* still freeing memory.
*/
@@ -472,8 +471,8 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
if (!has_intersects_mems_allowed(child, nodemask))
continue;
- /* badness() returns 0 if the thread is unkillable */
- child_points = badness(child, uptime.tv_sec);
+ /* oom_badness() returns 0 if the thread is unkillable */
+ child_points = oom_badness(child, uptime.tv_sec);
if (child_points > victim_points) {
victim = child;
victim_points = child_points;
--
1.6.5.2
Now, select_bad_process() have PF_KTHREAD check, but oom_kill_process
doesn't. It mean oom_kill_process() may choose wrong task, especially,
when the child are using use_mm().
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1969cc1..6ca6cb8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -466,6 +466,8 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
if (child->mm == p->mm)
continue;
+ if (child->flags & PF_KTHREAD)
+ continue;
if (mem && !task_in_mem_cgroup(child, mem))
continue;
if (!has_intersects_mems_allowed(child, nodemask))
--
1.6.5.2
When oom_kill_allocating_task is enabled, an argument of
oom_kill_process is not selected by select_bad_process(), but
just out_of_memory() caller task. It mean the task can be
unkillable. check it first.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 6ca6cb8..3e48023 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -436,6 +436,17 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
unsigned long victim_points = 0;
struct timespec uptime;
+ /*
+ * When oom_kill_allocating_task is enabled, p can be
+ * unkillable. check it first.
+ */
+ if (is_global_init(p) || (p->flags & PF_KTHREAD))
+ return 1;
+ if (mem && !task_in_mem_cgroup(p, mem))
+ return 1;
+ if (!has_intersects_mems_allowed(p, nodemask))
+ return 1;
+
if (printk_ratelimit())
dump_header(p, gfp_mask, order, mem);
--
1.6.5.2
Now, we have the same task check in three places. Unify it.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 39 +++++++++++++++++++++++----------------
1 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 3e48023..12204c7 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -276,6 +276,26 @@ static enum oom_constraint constrained_alloc(struct zonelist *zonelist,
}
#endif
+/* return true if the task is not adequate as candidate victim task. */
+static bool oom_unkillable_task(struct task_struct *p, struct mem_cgroup *mem,
+ const nodemask_t *nodemask)
+{
+ if (is_global_init(p))
+ return true;
+ if (p->flags & PF_KTHREAD)
+ return true;
+
+ /* When mem_cgroup_out_of_memory() and p is not member of the group */
+ if (mem && !task_in_mem_cgroup(p, mem))
+ return true;
+
+ /* p may not have freeable memory in nodemask */
+ if (!has_intersects_mems_allowed(p, nodemask))
+ return true;
+
+ return false;
+}
+
/*
* Simple selection loop. We chose the process with the highest
* number of 'points'. We expect the caller will lock the tasklist.
@@ -294,12 +314,7 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
for_each_process(p) {
unsigned long points;
- /* skip the init task and kthreads */
- if (is_global_init(p) || (p->flags & PF_KTHREAD))
- continue;
- if (mem && !task_in_mem_cgroup(p, mem))
- continue;
- if (!has_intersects_mems_allowed(p, nodemask))
+ if (oom_unkillable_task(p, mem, nodemask))
continue;
/*
@@ -440,11 +455,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
* When oom_kill_allocating_task is enabled, p can be
* unkillable. check it first.
*/
- if (is_global_init(p) || (p->flags & PF_KTHREAD))
- return 1;
- if (mem && !task_in_mem_cgroup(p, mem))
- return 1;
- if (!has_intersects_mems_allowed(p, nodemask))
+ if (oom_unkillable_task(p, mem, nodemask))
return 1;
if (printk_ratelimit())
@@ -477,11 +488,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
if (child->mm == p->mm)
continue;
- if (child->flags & PF_KTHREAD)
- continue;
- if (mem && !task_in_mem_cgroup(child, mem))
- continue;
- if (!has_intersects_mems_allowed(child, nodemask))
+ if (oom_unkillable_task(child, mem, nodemask))
continue;
/* oom_badness() returns 0 if the thread is unkillable */
--
1.6.5.2
Now, oom are using "child->mm != p->mm" check to distinguish subthread.
But It's incorrect. vfork() child also have the same ->mm.
This patch change to use same_thread_group() instead.
Cc: Oleg Nesterov <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 12204c7..e4b1146 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
list_for_each_entry(c, &t->children, sibling) {
child = find_lock_task_mm(c);
if (child) {
- if (child->mm != p->mm)
+ if (same_thread_group(p, child))
points += child->mm->total_vm/2 + 1;
task_unlock(child);
}
@@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
list_for_each_entry(child, &t->children, sibling) {
unsigned long child_points;
- if (child->mm == p->mm)
+ if (same_thread_group(p, child))
continue;
if (oom_unkillable_task(child, mem, nodemask))
continue;
--
1.6.5.2
Now, CAP_SYS_RAWIO check is very strange. if the user have both
CAP_SYS_ADMIN and CAP_SYS_RAWIO, points will makes 1/16.
Superuser's 1/4 bonus worthness is quite a bit dubious, but
considerable. However 1/16 is obviously insane.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 17 ++++++-----------
1 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index e4b1146..4236d39 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -198,19 +198,14 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
/*
* Superuser processes are usually more important, so we make it
- * less likely that we kill those.
+ * less likely that we kill those. And we don't want to kill a
+ * process with direct hardware access. Not only could that mess
+ * up the hardware, but usually users tend to only have this
+ * flag set on applications they think of as important.
*/
if (has_capability_noaudit(p, CAP_SYS_ADMIN) ||
- has_capability_noaudit(p, CAP_SYS_RESOURCE))
- points /= 4;
-
- /*
- * We don't want to kill a process with direct hardware access.
- * Not only could that mess up the hardware, but usually users
- * tend to only have this flag set on applications they think
- * of as important.
- */
- if (has_capability_noaudit(p, CAP_SYS_RAWIO))
+ has_capability_noaudit(p, CAP_SYS_RESOURCE) ||
+ has_capability_noaudit(p, CAP_SYS_RAWIO))
points /= 4;
/*
--
1.6.5.2
Now has_intersects_mems_allowed() has own thread iterate logic, but
it should use while_each_thread().
It slightly improve the code readability.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 4236d39..7e9942d 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -46,10 +46,10 @@ static DEFINE_SPINLOCK(zone_scan_lock);
* shares the same mempolicy nodes as current if it is bound by such a policy
* and whether or not it has the same set of allowed cpuset nodes.
*/
-static bool has_intersects_mems_allowed(struct task_struct *tsk,
+static bool has_intersects_mems_allowed(struct task_struct *p,
const nodemask_t *mask)
{
- struct task_struct *start = tsk;
+ struct task_struct *tsk = p;
do {
if (mask) {
@@ -69,8 +69,8 @@ static bool has_intersects_mems_allowed(struct task_struct *tsk,
if (cpuset_mems_allowed_intersects(current, tsk))
return true;
}
- tsk = next_thread(tsk);
- } while (tsk != start);
+ } while_each_thread(p, tsk);
+
return false;
}
#else
--
1.6.5.2
From: Luis Claudio R. Goncalves <[email protected]>
In a system under heavy load it was observed that even after the
oom-killer selects a task to die, the task may take a long time to die.
Right after sending a SIGKILL to the task selected by the oom-killer
this task has it's priority increased so that it can exit() exit soon,
freeing memory. That is accomplished by:
/*
* We give our sacrificial lamb high priority and access to
* all the memory it needs. That way it should be able to
* exit() and clear out its resources quickly...
*/
p->rt.time_slice = HZ;
set_tsk_thread_flag(p, TIF_MEMDIE);
It sounds plausible giving the dying task an even higher priority to be
sure it will be scheduled sooner and free the desired memory. It was
suggested on LKML using SCHED_FIFO:1, the lowest RT priority so that
this task won't interfere with any running RT task.
If the dying task is already an RT task, leave it untouched.
Another good suggestion, implemented here, was to avoid boosting the
dying task priority in case of mem_cgroup OOM.
Signed-off-by: Luis Claudio R. Goncalves <[email protected]>
Cc: Minchan Kim <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/oom_kill.c | 38 +++++++++++++++++++++++++++++++++++---
1 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7e9942d..1ecfc7a 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -82,6 +82,28 @@ static bool has_intersects_mems_allowed(struct task_struct *tsk,
#endif /* CONFIG_NUMA */
/*
+ * If this is a system OOM (not a memcg OOM) and the task selected to be
+ * killed is not already running at high (RT) priorities, speed up the
+ * recovery by boosting the dying task to the lowest FIFO priority.
+ * That helps with the recovery and avoids interfering with RT tasks.
+ */
+static void boost_dying_task_prio(struct task_struct *p,
+ struct mem_cgroup *mem)
+{
+ struct sched_param param = { .sched_priority = 1 };
+
+ if (mem)
+ return;
+
+ if (rt_task(p)) {
+ p->rt.time_slice = HZ;
+ return;
+ }
+
+ sched_setscheduler_nocheck(p, SCHED_FIFO, ¶m);
+}
+
+/*
* The process p may have detached its own ->mm while exiting or through
* use_mm(), but one or more of its subthreads may still have a valid
* pointer. Return p, or any of its subthreads with a valid ->mm, with
@@ -416,7 +438,7 @@ static void dump_header(struct task_struct *p, gfp_t gfp_mask, int order,
}
#define K(x) ((x) << (PAGE_SHIFT-10))
-static int oom_kill_task(struct task_struct *p)
+static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
{
p = find_lock_task_mm(p);
if (!p || p->signal->oom_adj == OOM_DISABLE) {
@@ -429,9 +451,17 @@ static int oom_kill_task(struct task_struct *p)
K(get_mm_counter(p->mm, MM_FILEPAGES)));
task_unlock(p);
- p->rt.time_slice = HZ;
+
set_tsk_thread_flag(p, TIF_MEMDIE);
force_sig(SIGKILL, p);
+
+ /*
+ * We give our sacrificial lamb high priority and access to
+ * all the memory it needs. That way it should be able to
+ * exit() and clear out its resources quickly...
+ */
+ boost_dying_task_prio(p, mem);
+
return 0;
}
#undef K
@@ -462,6 +492,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
*/
if (p->flags & PF_EXITING) {
set_tsk_thread_flag(p, TIF_MEMDIE);
+ boost_dying_task_prio(p, mem);
return 0;
}
@@ -495,7 +526,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
}
} while_each_thread(p, t);
- return oom_kill_task(victim);
+ return oom_kill_task(victim, mem);
}
/*
@@ -676,6 +707,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
*/
if (fatal_signal_pending(current)) {
set_thread_flag(TIF_MEMDIE);
+ boost_dying_task_prio(current, NULL);
return;
}
--
1.6.5.2
On 06/16, KOSAKI Motohiro wrote:
>
> Now, oom are using "child->mm != p->mm" check to distinguish subthread.
Heh. is it true??? I never undestood what oom_kill_process()->list_for_each_entry()
is supposed to do.
> But It's incorrect. vfork() child also have the same ->mm.
Yes.
> This patch change to use same_thread_group() instead.
I don't think we need same_thread_group(). Please note that any children must
be from the different thread_group.
So,
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
> list_for_each_entry(c, &t->children, sibling) {
> child = find_lock_task_mm(c);
> if (child) {
> - if (child->mm != p->mm)
> + if (same_thread_group(p, child))
> points += child->mm->total_vm/2 + 1;
> task_unlock(child);
> }
> @@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> list_for_each_entry(child, &t->children, sibling) {
> unsigned long child_points;
>
> - if (child->mm == p->mm)
> + if (same_thread_group(p, child))
> continue;
In both cases same_thread_group() must be false.
This means that the change in oom_badness() doesn't look right,
"child->mm != p->mm" is the correct check to decide whether we should
account child->mm.
The change in oom_kill_process() merely removes this "continue".
Could someone please explain what this code _should_ do?
Oleg.
On Wed, Jun 16, 2010 at 08:29:13PM +0900, KOSAKI Motohiro wrote:
> Now, badness() doesn't care neigher CPUSET nor mempolicy. Then
> if the victim child process have disjoint nodemask, __out_of_memory()
> can makes kernel hang eventually.
>
> This patch fixes it.
>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Reviewed-by: Minchan Kim <[email protected]>
This patch inclues two things.
1. consider cpuset and mempolicy in oom_kill_process
2. Simplify mempolicy oom check with nodemask != NULL
in select_bad_process.
1) change behavior but 2) is just cleanup.
It should have been in another patch to reivew easily. :)
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:31:20PM +0900, KOSAKI Motohiro wrote:
>
> badness() is wrong name because it's too generic name. rename it.
>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Reviewed-by: Minchan Kim <[email protected]>
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:32:08PM +0900, KOSAKI Motohiro wrote:
> Now, select_bad_process() have PF_KTHREAD check, but oom_kill_process
> doesn't. It mean oom_kill_process() may choose wrong task, especially,
> when the child are using use_mm().
Now oom_kill_process is called by three place.
1. mem_cgroup_out_of_memory
2. out_of_memory with sysctl_oom_kill_allocating_task
3. out_of_memory with non-sysctl_oom_kill_allocating_task
I think it's no problem in 1 and 3 since select_bad_process already checks
PF_KTHREAD. The problem in in 2.
So How about put the check before calling oom_kill_process in case of
sysctl_oom_kill_allocating task?
if (sysctl_oom_kill_allocating_task) {
if (!current->flags & PF_KTHREAD)
oom_kill_process();
It can remove duplicated PF_KTHREAD check in select_bad_process and
oom_kill_process.
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:32:45PM +0900, KOSAKI Motohiro wrote:
> When oom_kill_allocating_task is enabled, an argument of
> oom_kill_process is not selected by select_bad_process(), but
> just out_of_memory() caller task. It mean the task can be
> unkillable. check it first.
>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
> ---
> mm/oom_kill.c | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 6ca6cb8..3e48023 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -436,6 +436,17 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> unsigned long victim_points = 0;
> struct timespec uptime;
>
> + /*
> + * When oom_kill_allocating_task is enabled, p can be
> + * unkillable. check it first.
> + */
> + if (is_global_init(p) || (p->flags & PF_KTHREAD))
> + return 1;
> + if (mem && !task_in_mem_cgroup(p, mem))
> + return 1;
> + if (!has_intersects_mems_allowed(p, nodemask))
> + return 1;
> +
I think this check could be done before oom_kill_proces in case of
sysctl_oom_kill_allocating_task, too.
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:33:17PM +0900, KOSAKI Motohiro wrote:
>
> Now, we have the same task check in three places. Unify it.
>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Reviewed-by: Minchan Kim <[email protected]>
But please consider my previous comment.
If I am not wrong, we need change this patch, too.
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:34:02PM +0900, KOSAKI Motohiro wrote:
> Now, oom are using "child->mm != p->mm" check to distinguish subthread.
> But It's incorrect. vfork() child also have the same ->mm.
>
> This patch change to use same_thread_group() instead.
Hmm. I think we don't use it to distinguish subthread.
We use it for finding child process which is not vforked.
I can't understand your point.
>
> Cc: Oleg Nesterov <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
> ---
> mm/oom_kill.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 12204c7..e4b1146 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
> list_for_each_entry(c, &t->children, sibling) {
> child = find_lock_task_mm(c);
> if (child) {
> - if (child->mm != p->mm)
> + if (same_thread_group(p, child))
> points += child->mm->total_vm/2 + 1;
> task_unlock(child);
> }
> @@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> list_for_each_entry(child, &t->children, sibling) {
> unsigned long child_points;
>
> - if (child->mm == p->mm)
> + if (same_thread_group(p, child))
> continue;
> if (oom_unkillable_task(child, mem, nodemask))
> continue;
> --
> 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>
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:35:15PM +0900, KOSAKI Motohiro wrote:
>
> Now has_intersects_mems_allowed() has own thread iterate logic, but
> it should use while_each_thread().
>
> It slightly improve the code readability.
>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Reviewed-by: Minchan Kim <[email protected]>
--
Kind regards,
Minchan Kim
On Wed, Jun 16, 2010 at 08:36:29PM +0900, KOSAKI Motohiro wrote:
>
> From: Luis Claudio R. Goncalves <[email protected]>
>
> In a system under heavy load it was observed that even after the
> oom-killer selects a task to die, the task may take a long time to die.
>
> Right after sending a SIGKILL to the task selected by the oom-killer
> this task has it's priority increased so that it can exit() exit soon,
> freeing memory. That is accomplished by:
>
> /*
> * We give our sacrificial lamb high priority and access to
> * all the memory it needs. That way it should be able to
> * exit() and clear out its resources quickly...
> */
> p->rt.time_slice = HZ;
> set_tsk_thread_flag(p, TIF_MEMDIE);
>
> It sounds plausible giving the dying task an even higher priority to be
> sure it will be scheduled sooner and free the desired memory. It was
> suggested on LKML using SCHED_FIFO:1, the lowest RT priority so that
> this task won't interfere with any running RT task.
>
> If the dying task is already an RT task, leave it untouched.
> Another good suggestion, implemented here, was to avoid boosting the
> dying task priority in case of mem_cgroup OOM.
>
> Signed-off-by: Luis Claudio R. Goncalves <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
> ---
> mm/oom_kill.c | 38 +++++++++++++++++++++++++++++++++++---
> 1 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7e9942d..1ecfc7a 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -82,6 +82,28 @@ static bool has_intersects_mems_allowed(struct task_struct *tsk,
> #endif /* CONFIG_NUMA */
>
> /*
> + * If this is a system OOM (not a memcg OOM) and the task selected to be
> + * killed is not already running at high (RT) priorities, speed up the
> + * recovery by boosting the dying task to the lowest FIFO priority.
> + * That helps with the recovery and avoids interfering with RT tasks.
> + */
> +static void boost_dying_task_prio(struct task_struct *p,
> + struct mem_cgroup *mem)
> +{
> + struct sched_param param = { .sched_priority = 1 };
> +
> + if (mem)
> + return;
> +
> + if (rt_task(p)) {
> + p->rt.time_slice = HZ;
> + return;
I have a question from long time ago.
If we change rt.time_slice _without_ setscheduler, is it effective?
I mean scheduler pick up the task faster than other normal task?
> + }
> +
> + sched_setscheduler_nocheck(p, SCHED_FIFO, ¶m);
> +}
> +
> +/*
--
Kind regards,
Minchan Kim
On Thu, Jun 17, 2010 at 12:31:20AM +0900, Minchan Kim wrote:
| > /*
| > * We give our sacrificial lamb high priority and access to
| > * all the memory it needs. That way it should be able to
| > * exit() and clear out its resources quickly...
| > */
| > p->rt.time_slice = HZ;
| > set_tsk_thread_flag(p, TIF_MEMDIE);
...
| > + if (rt_task(p)) {
| > + p->rt.time_slice = HZ;
| > + return;
I am not sure the code above will have any real effect for an RT task.
Kosaki-san, was this change motivated by test results or was it just a code
cleanup? I ask that out of curiosity.
| I have a question from long time ago.
| If we change rt.time_slice _without_ setscheduler, is it effective?
| I mean scheduler pick up the task faster than other normal task?
$ git log --pretty=oneline -Stime_slice mm/oom_kill.c
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
This code ("time_slice = HZ;") is around for quite a while and
probably comes from a time where having a big time slice was enough to be
sure you would be the next on the line. I would say sched_setscheduler is
indeed necessary.
Regards,
Luis
--
[ Luis Claudio R. Goncalves Red Hat - Realtime Team ]
[ Fingerprint: 4FDD B8C4 3C59 34BD 8BE9 2696 7203 D980 A448 C8F8 ]
On Wed, 16 Jun 2010, KOSAKI Motohiro wrote:
>
> badness() is wrong name because it's too generic name. rename it.
>
This is already done in my badness heuristic rewrite, can we please focus
on its review?
> On Wed, Jun 16, 2010 at 08:32:08PM +0900, KOSAKI Motohiro wrote:
> > Now, select_bad_process() have PF_KTHREAD check, but oom_kill_process
> > doesn't. It mean oom_kill_process() may choose wrong task, especially,
> > when the child are using use_mm().
> Now oom_kill_process is called by three place.
>
> 1. mem_cgroup_out_of_memory
> 2. out_of_memory with sysctl_oom_kill_allocating_task
> 3. out_of_memory with non-sysctl_oom_kill_allocating_task
>
> I think it's no problem in 1 and 3 since select_bad_process already checks
> PF_KTHREAD. The problem in in 2.
> So How about put the check before calling oom_kill_process in case of
> sysctl_oom_kill_allocating task?
>
> if (sysctl_oom_kill_allocating_task) {
> if (!current->flags & PF_KTHREAD)
> oom_kill_process();
>
> It can remove duplicated PF_KTHREAD check in select_bad_process and
> oom_kill_process.
This patch changed child selection logic. select_bad_process() doesn't
check victim's child. IOW, this is necessary when all 1-3.
> On Wed, Jun 16, 2010 at 08:34:02PM +0900, KOSAKI Motohiro wrote:
> > Now, oom are using "child->mm != p->mm" check to distinguish subthread.
> > But It's incorrect. vfork() child also have the same ->mm.
> >
> > This patch change to use same_thread_group() instead.
>
> Hmm. I think we don't use it to distinguish subthread.
> We use it for finding child process which is not vforked.
>
> I can't understand your point.
Thank you. my fault. respin.
> On 06/16, KOSAKI Motohiro wrote:
> >
> > Now, oom are using "child->mm != p->mm" check to distinguish subthread.
>
> Heh. is it true??? I never undestood what oom_kill_process()->list_for_each_entry()
> is supposed to do.
I guessed. true history was gone long time ago ;)
ok, I'll remove dubious guess.
> > But It's incorrect. vfork() child also have the same ->mm.
>
> Yes.
>
> > This patch change to use same_thread_group() instead.
>
> I don't think we need same_thread_group(). Please note that any children must
> be from the different thread_group.
Agghh. I see.
ok, probably, I've got correct original author intention now.
To be honest, andrea's ancient patch is very hard to understand for me ;)
>
> So,
>
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -161,7 +161,7 @@ unsigned long oom_badness(struct task_struct *p, unsigned long uptime)
> > list_for_each_entry(c, &t->children, sibling) {
> > child = find_lock_task_mm(c);
> > if (child) {
> > - if (child->mm != p->mm)
> > + if (same_thread_group(p, child))
> > points += child->mm->total_vm/2 + 1;
> > task_unlock(child);
> > }
> > @@ -486,7 +486,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > list_for_each_entry(child, &t->children, sibling) {
> > unsigned long child_points;
> >
> > - if (child->mm == p->mm)
> > + if (same_thread_group(p, child))
> > continue;
>
> In both cases same_thread_group() must be false.
>
> This means that the change in oom_badness() doesn't look right,
> "child->mm != p->mm" is the correct check to decide whether we should
> account child->mm.
>
> The change in oom_kill_process() merely removes this "continue".
> Could someone please explain what this code _should_ do?
I think you are right.
> On Thu, Jun 17, 2010 at 12:31:20AM +0900, Minchan Kim wrote:
> | > /*
> | > * We give our sacrificial lamb high priority and access to
> | > * all the memory it needs. That way it should be able to
> | > * exit() and clear out its resources quickly...
> | > */
> | > p->rt.time_slice = HZ;
> | > set_tsk_thread_flag(p, TIF_MEMDIE);
> ...
> | > + if (rt_task(p)) {
> | > + p->rt.time_slice = HZ;
> | > + return;
>
> I am not sure the code above will have any real effect for an RT task.
> Kosaki-san, was this change motivated by test results or was it just a code
> cleanup? I ask that out of curiosity.
just cleanup.
ok, I remove this dubious code.
>
> | I have a question from long time ago.
> | If we change rt.time_slice _without_ setscheduler, is it effective?
> | I mean scheduler pick up the task faster than other normal task?
>
> $ git log --pretty=oneline -Stime_slice mm/oom_kill.c
> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
>
> This code ("time_slice = HZ;") is around for quite a while and
> probably comes from a time where having a big time slice was enough to be
> sure you would be the next on the line. I would say sched_setscheduler is
> indeed necessary.
ok
> On Wed, Jun 16, 2010 at 08:29:13PM +0900, KOSAKI Motohiro wrote:
> > Now, badness() doesn't care neigher CPUSET nor mempolicy. Then
> > if the victim child process have disjoint nodemask, __out_of_memory()
> > can makes kernel hang eventually.
> >
> > This patch fixes it.
> >
> > Signed-off-by: KOSAKI Motohiro <[email protected]>
> Reviewed-by: Minchan Kim <[email protected]>
>
> This patch inclues two things.
>
> 1. consider cpuset and mempolicy in oom_kill_process
> 2. Simplify mempolicy oom check with nodemask != NULL
> in select_bad_process.
>
> 1) change behavior but 2) is just cleanup.
> It should have been in another patch to reivew easily. :)
Thank you. removed (2).
> On Wed, 16 Jun 2010, KOSAKI Motohiro wrote:
>
> >
> > badness() is wrong name because it's too generic name. rename it.
> >
>
> This is already done in my badness heuristic rewrite, can we please focus
> on its review?
Please resend that as individual patches.
> On Wed, Jun 16, 2010 at 08:32:45PM +0900, KOSAKI Motohiro wrote:
> > When oom_kill_allocating_task is enabled, an argument of
> > oom_kill_process is not selected by select_bad_process(), but
> > just out_of_memory() caller task. It mean the task can be
> > unkillable. check it first.
> >
> > Signed-off-by: KOSAKI Motohiro <[email protected]>
> > ---
> > mm/oom_kill.c | 11 +++++++++++
> > 1 files changed, 11 insertions(+), 0 deletions(-)
> >
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 6ca6cb8..3e48023 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -436,6 +436,17 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > unsigned long victim_points = 0;
> > struct timespec uptime;
> >
> > + /*
> > + * When oom_kill_allocating_task is enabled, p can be
> > + * unkillable. check it first.
> > + */
> > + if (is_global_init(p) || (p->flags & PF_KTHREAD))
> > + return 1;
> > + if (mem && !task_in_mem_cgroup(p, mem))
> > + return 1;
> > + if (!has_intersects_mems_allowed(p, nodemask))
> > + return 1;
> > +
>
> I think this check could be done before oom_kill_proces in case of
> sysctl_oom_kill_allocating_task, too.
ok.
> > + struct sched_param param = { .sched_priority = 1 };
> > +
> > + if (mem)
> > + return;
> > +
> > + if (rt_task(p)) {
> > + p->rt.time_slice = HZ;
> > + return;
>
> I have a question from long time ago.
> If we change rt.time_slice _without_ setscheduler, is it effective?
> I mean scheduler pick up the task faster than other normal task?
if p is SCHED_OTHER, no effective. if my understand is correct, that's
only meaningfull if p is SCHED_RR. that's the reason why I moved this
check into "if (rt_task())".
but honestly I haven't observed this works effectively. so, I agree
this can be removed as Luis mentioned.