[patch] mm, oom: replace some information in tasklist dump

[patch] mm, oom: replace some information in tasklist dump

[David Rientjes]

The number of ptes and swap entries are used in the oom killer's badness
heuristic, so they should be shown in the tasklist dump.

This patch adds those fields and replaces cpu and oom_adj values that are
currently emitted.  Cpu isn't interesting and oom_adj is deprecated and
will be removed later this year, the same information is already
displayed as oom_score_adj which is used internally.

At the same time, make the documentation a little more clear to state
this information is helpful to determine why the oom killer chose the
task it did to kill.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 Documentation/sysctl/vm.txt |    7 ++++---
 mm/oom_kill.c               |   11 ++++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -502,9 +502,10 @@ oom_dump_tasks
 
 Enables a system-wide task dump (excluding kernel threads) to be
 produced when the kernel performs an OOM-killing and includes such
-information as pid, uid, tgid, vm size, rss, cpu, oom_adj score, and
-name.  This is helpful to determine why the OOM killer was invoked
-and to identify the rogue task that caused it.
+information as pid, uid, tgid, vm size, rss, nr_ptes, swapents,
+oom_score_adj score, and name.  This is helpful to determine why the
+OOM killer was invoked, to identify the rogue task that caused it,
+and to determine why the OOM killer chose the task it did to kill.
 
 If this is set to zero, this information is suppressed.  On very
 large systems with thousands of tasks it may not be feasible to dump
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -371,8 +371,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
  * Dumps the current memory state of all eligible tasks.  Tasks not in the same
  * memcg, not in the same cpuset, or bound to a disjoint set of mempolicy nodes
  * are not shown.
- * State information includes task's pid, uid, tgid, vm size, rss, cpu, oom_adj
- * value, oom_score_adj value, and name.
+ * State information includes task's pid, uid, tgid, vm size, rss, nr_ptes,
+ * swapents, oom_score_adj value, and name.
  *
  * Call with tasklist_lock read-locked.
  */
@@ -381,7 +381,7 @@ static void dump_tasks(const struct mem_cgroup *memcg, const nodemask_t *nodemas
 	struct task_struct *p;
 	struct task_struct *task;
 
-	pr_info("[ pid ]   uid  tgid total_vm      rss cpu oom_adj oom_score_adj name\n");
+	pr_info("[ pid ]   uid  tgid total_vm      rss nr_ptes swapents oom_score_adj name\n");
 	for_each_process(p) {
 		if (oom_unkillable_task(p, memcg, nodemask))
 			continue;
@@ -396,10 +396,11 @@ static void dump_tasks(const struct mem_cgroup *memcg, const nodemask_t *nodemas
 			continue;
 		}
 
-		pr_info("[%5d] %5d %5d %8lu %8lu %3u     %3d         %5d %s\n",
+		pr_info("[%5d] %5d %5d %8lu %8lu %7lu %8lu         %5d %s\n",
 			task->pid, from_kuid(&init_user_ns, task_uid(task)),
 			task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
-			task_cpu(task), task->signal->oom_adj,
+			task->mm->nr_ptes,
+			get_mm_counter(task->mm, MM_SWAPENTS),
 			task->signal->oom_score_adj, task->comm);
 		task_unlock(task);
 	}

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

Re: [patch] mm, oom: replace some information in tasklist dump

[KOSAKI Motohiro]

On Fri, Jun 22, 2012 at 5:45 PM, David Rientjes <rientjes@google.com> wrote:
> The number of ptes and swap entries are used in the oom killer's badness
> heuristic, so they should be shown in the tasklist dump.
>
> This patch adds those fields and replaces cpu and oom_adj values that are
> currently emitted.  Cpu isn't interesting and oom_adj is deprecated and
> will be removed later this year, the same information is already
> displayed as oom_score_adj which is used internally.
>
> At the same time, make the documentation a little more clear to state
> this information is helpful to determine why the oom killer chose the
> task it did to kill.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  Documentation/sysctl/vm.txt |    7 ++++---
>  mm/oom_kill.c               |   11 ++++++-----
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
> --- a/Documentation/sysctl/vm.txt
> +++ b/Documentation/sysctl/vm.txt
> @@ -502,9 +502,10 @@ oom_dump_tasks
>
>  Enables a system-wide task dump (excluding kernel threads) to be
>  produced when the kernel performs an OOM-killing and includes such
> -information as pid, uid, tgid, vm size, rss, cpu, oom_adj score, and
> -name.  This is helpful to determine why the OOM killer was invoked
> -and to identify the rogue task that caused it.
> +information as pid, uid, tgid, vm size, rss, nr_ptes, swapents,
> +oom_score_adj score, and name.  This is helpful to determine why the
> +OOM killer was invoked, to identify the rogue task that caused it,
> +and to determine why the OOM killer chose the task it did to kill.
>
>  If this is set to zero, this information is suppressed.  On very
>  large systems with thousands of tasks it may not be feasible to dump
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -371,8 +371,8 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
>  * Dumps the current memory state of all eligible tasks.  Tasks not in the same
>  * memcg, not in the same cpuset, or bound to a disjoint set of mempolicy nodes
>  * are not shown.
> - * State information includes task's pid, uid, tgid, vm size, rss, cpu, oom_adj
> - * value, oom_score_adj value, and name.
> + * State information includes task's pid, uid, tgid, vm size, rss, nr_ptes,
> + * swapents, oom_score_adj value, and name.
>  *
>  * Call with tasklist_lock read-locked.
>  */
> @@ -381,7 +381,7 @@ static void dump_tasks(const struct mem_cgroup *memcg, const nodemask_t *nodemas
>        struct task_struct *p;
>        struct task_struct *task;
>
> -       pr_info("[ pid ]   uid  tgid total_vm      rss cpu oom_adj oom_score_adj name\n");
> +       pr_info("[ pid ]   uid  tgid total_vm      rss nr_ptes swapents oom_score_adj name\n");
>        for_each_process(p) {
>                if (oom_unkillable_task(p, memcg, nodemask))
>                        continue;
> @@ -396,10 +396,11 @@ static void dump_tasks(const struct mem_cgroup *memcg, const nodemask_t *nodemas
>                        continue;
>                }
>
> -               pr_info("[%5d] %5d %5d %8lu %8lu %3u     %3d         %5d %s\n",
> +               pr_info("[%5d] %5d %5d %8lu %8lu %7lu %8lu         %5d %s\n",
>                        task->pid, from_kuid(&init_user_ns, task_uid(task)),
>                        task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
> -                       task_cpu(task), task->signal->oom_adj,
> +                       task->mm->nr_ptes,

nr_ptes should be folded into rss. it's "resident".
btw, /proc rss info should be fixed too.



> +                       get_mm_counter(task->mm, MM_SWAPENTS),
>                        task->signal->oom_score_adj, task->comm);
>                task_unlock(task);
>        }
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

Re: [patch] mm, oom: replace some information in tasklist dump

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 909 bytes --]

On Fri, 22 Jun 2012, KOSAKI Motohiro wrote:

> > @@ -396,10 +396,11 @@ static void dump_tasks(const struct mem_cgroup *memcg, const nodemask_t *nodemas
> >                        continue;
> >                }
> >
> > -               pr_info("[%5d] %5d %5d %8lu %8lu %3u     %3d         %5d %s\n",
> > +               pr_info("[%5d] %5d %5d %8lu %8lu %7lu %8lu         %5d %s\n",
> >                        task->pid, from_kuid(&init_user_ns, task_uid(task)),
> >                        task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
> > -                       task_cpu(task), task->signal->oom_adj,
> > +                       task->mm->nr_ptes,
> 
> nr_ptes should be folded into rss. it's "resident".
> btw, /proc rss info should be fixed too.
> 

If we can fold rss into get_mm_rss() and every caller is ok with that, 
then we can remove showing it here and adding it explicitly in 
oom_badness().

Re: [patch] mm, oom: replace some information in tasklist dump

[KOSAKI Motohiro]

>> > -               pr_info("[%5d] %5d %5d %8lu %8lu %3u     %3d         %5d %s\n",
>> > +               pr_info("[%5d] %5d %5d %8lu %8lu %7lu %8lu         %5d %s\n",
>> >                        task->pid, from_kuid(&init_user_ns, task_uid(task)),
>> >                        task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
>> > -                       task_cpu(task), task->signal->oom_adj,
>> > +                       task->mm->nr_ptes,
>>
>> nr_ptes should be folded into rss. it's "resident".
>> btw, /proc rss info should be fixed too.
>
> If we can fold rss into get_mm_rss() and every caller is ok with that,
> then we can remove showing it here and adding it explicitly in
> oom_badness().

No worth to make fragile ABI. Do you have any benefit?

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

Re: [patch] mm, oom: replace some information in tasklist dump

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1147 bytes --]

On Fri, 22 Jun 2012, KOSAKI Motohiro wrote:

> >> > -               pr_info("[%5d] %5d %5d %8lu %8lu %3u     %3d         %5d %s\n",
> >> > +               pr_info("[%5d] %5d %5d %8lu %8lu %7lu %8lu         %5d %s\n",
> >> >                        task->pid, from_kuid(&init_user_ns, task_uid(task)),
> >> >                        task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
> >> > -                       task_cpu(task), task->signal->oom_adj,
> >> > +                       task->mm->nr_ptes,
> >>
> >> nr_ptes should be folded into rss. it's "resident".
> >> btw, /proc rss info should be fixed too.
> >
> > If we can fold rss into get_mm_rss() and every caller is ok with that,
> > then we can remove showing it here and adding it explicitly in
> > oom_badness().
> 
> No worth to make fragile ABI. Do you have any benefit?
> 

Yes, because this is exactly where we would discover something like a 
mm->nr_ptes accounting issue since it would result in an oom kill and we'd 
notice the mismatch between nr_ptes and rss in the tasklist dump.

I also don't think anybody is parsing this tasklist dump or I wouldn't be 
changing it here.

Re: [patch] mm, oom: replace some information in tasklist dump

[KOSAKI Motohiro]

(6/22/12 7:36 PM), David Rientjes wrote:
> On Fri, 22 Jun 2012, KOSAKI Motohiro wrote:
> 
>>>>> -               pr_info("[%5d] %5d %5d %8lu %8lu %3u     %3d         %5d %s\n",
>>>>> +               pr_info("[%5d] %5d %5d %8lu %8lu %7lu %8lu         %5d %s\n",
>>>>>                        task->pid, from_kuid(&init_user_ns, task_uid(task)),
>>>>>                        task->tgid, task->mm->total_vm, get_mm_rss(task->mm),
>>>>> -                       task_cpu(task), task->signal->oom_adj,
>>>>> +                       task->mm->nr_ptes,
>>>>
>>>> nr_ptes should be folded into rss. it's "resident".
>>>> btw, /proc rss info should be fixed too.
>>>
>>> If we can fold rss into get_mm_rss() and every caller is ok with that,
>>> then we can remove showing it here and adding it explicitly in
>>> oom_badness().
>>
>> No worth to make fragile ABI. Do you have any benefit?
>>
> 
> Yes, because this is exactly where we would discover something like a 
> mm->nr_ptes accounting issue since it would result in an oom kill and we'd 
> notice the mismatch between nr_ptes and rss in the tasklist dump.

Below patch is better, then. tasklist dump should show brief summary and
final killed process output should show most detail info. And, now all of
get_mm_rss() callsite got consistent.



---
 arch/sparc/mm/fault_64.c |    3 ++-
 arch/sparc/mm/tsb.c      |    3 ++-
 include/linux/mm.h       |    3 ++-
 mm/oom_kill.c            |    8 ++++----
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c
index 1fe0429..f342a7c 100644
--- a/arch/sparc/mm/fault_64.c
+++ b/arch/sparc/mm/fault_64.c
@@ -463,7 +463,8 @@ good_area:
 	}
 	up_read(&mm->mmap_sem);
 
-	mm_rss = get_mm_rss(mm);
+	mm_rss = get_mm_counter(mm, MM_FILEPAGES) + get_mm_counter(mm, MM_ANONPAGES);
+
 #ifdef CONFIG_HUGETLB_PAGE
 	mm_rss -= (mm->context.huge_pte_count * (HPAGE_SIZE / PAGE_SIZE));
 #endif
diff --git a/arch/sparc/mm/tsb.c b/arch/sparc/mm/tsb.c
index c52add7..20889dd 100644
--- a/arch/sparc/mm/tsb.c
+++ b/arch/sparc/mm/tsb.c
@@ -472,7 +472,8 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
 	/* If this is fork, inherit the parent's TSB size.  We would
 	 * grow it to that size on the first page fault anyways.
 	 */
-	tsb_grow(mm, MM_TSB_BASE, get_mm_rss(mm));
+	tsb_grow(mm, MM_TSB_BASE, get_mm_counter(mm, MM_FILEPAGES) +
+		 get_mm_counter(mm, MM_ANONPAGES));
 
 #ifdef CONFIG_HUGETLB_PAGE
 	if (unlikely(huge_pte_count))
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..c2b8d34 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1091,7 +1091,8 @@ static inline void dec_mm_counter(struct mm_struct *mm, int member)
 static inline unsigned long get_mm_rss(struct mm_struct *mm)
 {
 	return get_mm_counter(mm, MM_FILEPAGES) +
-		get_mm_counter(mm, MM_ANONPAGES);
+		get_mm_counter(mm, MM_ANONPAGES) +
+		mm->nr_ptes;
 }
 
 static inline unsigned long get_mm_hiwater_rss(struct mm_struct *mm)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ac300c9..c3fba3c 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -203,8 +203,7 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
 	 * The baseline for the badness score is the proportion of RAM that each
 	 * task's rss, pagetable and swap space use.
 	 */
-	points = get_mm_rss(p->mm) + p->mm->nr_ptes +
-		 get_mm_counter(p->mm, MM_SWAPENTS);
+	points = get_mm_rss(p->mm) + get_mm_counter(p->mm, MM_SWAPENTS);
 	task_unlock(p);
 
 	/*
@@ -483,10 +482,11 @@ static void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 
 	/* mm cannot safely be dereferenced after task_unlock(victim) */
 	mm = victim->mm;
-	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukBJPYn",
+	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB pte-rss:%lukBJPYn",
 		task_pid_nr(victim), victim->comm, K(victim->mm->total_vm),
 		K(get_mm_counter(victim->mm, MM_ANONPAGES)),
-		K(get_mm_counter(victim->mm, MM_FILEPAGES)));
+		K(get_mm_counter(victim->mm, MM_FILEPAGES)),
+		K(mm->nr_ptes));
 	task_unlock(victim);
 
 	/*
-- 
1.7.1

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

Re: [patch] mm, oom: replace some information in tasklist dump

[David Rientjes]

On Fri, 22 Jun 2012, KOSAKI Motohiro wrote:

> >> No worth to make fragile ABI. Do you have any benefit?
> >>
> > 
> > Yes, because this is exactly where we would discover something like a 
> > mm->nr_ptes accounting issue since it would result in an oom kill and we'd 
> > notice the mismatch between nr_ptes and rss in the tasklist dump.
> 
> Below patch is better, then. tasklist dump should show brief summary and
> final killed process output should show most detail info. And, now all of
> get_mm_rss() callsite got consistent.
> 

No, it's not.

Your patch is factoring ptes into get_mm_rss() throughout the kernel, my 
patch is showing get_mm_rss() and nr_ptes in the oom killer tasklist dump 
since they are both (currently) factored in seperately.  They are two 
functionally different changes.

If you want to factor ptes into get_mm_rss() and make that change 
throughout the kernel, then you should patch linux-next which includes my 
oom patch, write an actual changelog for why ptes should now be included 
in get_mm_rss() -- which I'll nack because it significantly changes 
/proc/pid/stat output for applications between kernel versions that we 
depend very heavily on -- and propose it seperately.

Thanks.

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

Re: [patch] mm, oom: replace some information in tasklist dump

[KOSAKI Motohiro]

(6/24/12 4:43 PM), David Rientjes wrote:
> On Fri, 22 Jun 2012, KOSAKI Motohiro wrote:
> 
>>>> No worth to make fragile ABI. Do you have any benefit?
>>>>
>>>
>>> Yes, because this is exactly where we would discover something like a 
>>> mm->nr_ptes accounting issue since it would result in an oom kill and we'd 
>>> notice the mismatch between nr_ptes and rss in the tasklist dump.
>>
>> Below patch is better, then. tasklist dump should show brief summary and
>> final killed process output should show most detail info. And, now all of
>> get_mm_rss() callsite got consistent.
>>
> 
> No, it's not.
> 
> Your patch is factoring ptes into get_mm_rss() throughout the kernel, my 
> patch is showing get_mm_rss() and nr_ptes in the oom killer tasklist dump 
> since they are both (currently) factored in seperately.  They are two 
> functionally different changes.

I said they should not showed separetly. That's all. Don't request talk the
same repeat.





> If you want to factor ptes into get_mm_rss() and make that change 
> throughout the kernel, then you should patch linux-next which includes my 
> oom patch, write an actual changelog for why ptes should now be included 
> in get_mm_rss() -- which I'll nack because it significantly changes 
> /proc/pid/stat output for applications between kernel versions that we 
> depend very heavily on -- and propose it seperately.


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

Re: [patch] mm, oom: replace some information in tasklist dump

[David Rientjes]

On Mon, 25 Jun 2012, KOSAKI Motohiro wrote:

> > Your patch is factoring ptes into get_mm_rss() throughout the kernel, my 
> > patch is showing get_mm_rss() and nr_ptes in the oom killer tasklist dump 
> > since they are both (currently) factored in seperately.  They are two 
> > functionally different changes.
> 
> I said they should not showed separetly. That's all. Don't request talk the
> same repeat.
> 

I'm sorry, but I don't understand what you're trying to say.  If you have 
a patch to build upon in -mm (since my patch is already in it), feel free 
to post it with a changelog.  Thanks!

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