proc: revert to show stack information in /proc/{pid}/status

Commit d899bf7b (procfs: provide stack information for threads) introduced to show stack information in /proc/{pid}/status. But it cause large performance regression. Unfortunately /proc/{pid}/status is used ps command too and ps is one of most important component. Because both to take mmap_sem and page table walk are heavily operation.

Thus, this patch revert it. Fortunately /proc/{pid}/task/{tid}/smaps provide almost same information. we can use it.

Cc Stefani Seibold <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---

Am Donnerstag, den 31.12.2009, 23:12 +0900 schrieb KOSAKI Motohiro:

Quoted 6 lines

Commit d899bf7b (procfs: provide stack information for threads) introduced to show stack information in /proc/{pid}/status. But it cause large performance regression. Unfortunately /proc/{pid}/status is used ps command too and ps is one of most important component. Because both to take mmap_sem and page table walk are heavily operation.

/proc/<pid>/status is IMHO not a performance relevant thing. The main reason is to provide exact information about a running process.

Quoted 2 lines

Thus, this patch revert it. Fortunately /proc/{pid}/task/{tid}/smaps provide almost same information. we can use it.

Completely wrong. Where does smaps provides this kind of information? Where is there the high water mark of the stack usage?

Quoted 8 lines

Am Donnerstag, den 31.12.2009, 23:12 +0900 schrieb KOSAKI Motohiro:

Quoted 6 lines

Commit d899bf7b (procfs: provide stack information for threads) introduced to show stack information in /proc/{pid}/status. But it cause large performance regression. Unfortunately /proc/{pid}/status is used ps command too and ps is one of most important component. Because both to take mmap_sem and page table walk are heavily operation.

Quoted 2 lines

/proc/<pid>/status is IMHO not a performance relevant thing. The main reason is to provide exact information about a running process.

No. You have to learn real world use case. if you think so, you should change ps before submit this change. This patch obviously make harm than worth. Nobody (except you) use it but everybody get regression.

Quoted 6 lines

Quoted 3 lines

Thus, this patch revert it. Fortunately /proc/{pid}/task/{tid}/smaps provide almost same information. we can use it.

Completely wrong. Where does smaps provides this kind of information? Where is there the high water mark of the stack usage?

You have to see you patch itself. show_map_vma() isn't only used by /proc/pid/maps, but also be used by /proc/pid/smaps.

Now, /proc/{pid}/task/{tid}/smaps show following column.

7fb97c181000-7fb97d1d1000 rw-p 00000000 00:00 0 [threadstack:0084eff0]

Size:              16704 kB
Rss:                   8 kB
Pss:                   8 kB
Shared_Clean:          0 kB
Shared_Dirty:          0 kB
Private_Clean:         0 kB
Private_Dirty:         8 kB
Referenced:            8 kB
Swap:                  0 kB
KernelPageSize:        4 kB
MMUPageSize:           4 kB

It descibe
- This vma is thread stack
- vma size is 16704kB
- stack vsz size in vma is 0x0084eff0 (~ 8508kB)
- and, physical memory is used 8kB.

Anyway, I revert the regresstion patch as other regression patches. if you really want this feature, you have three options.

1. create new /proc file instead to use /proc/pid/status.

2. improve performance until typical use-case don't notice regression.

3. change ps and other /proc related userland implementation and resubmit this patch.

But even if you choose anything, You have to test both its functional and performance _before_ submitting kernel patch.

But ps is not a performance critical application nor a cat /proc/*/status!

Quoted 2 lines · Stefani Seibold, le Fri 01 Jan 2010 16:10:16 +0100, a écrit :

But ps is not a performance critical application nor a cat /proc/*/status!

Errr, maybe not so much as some other operations, but a lot of tools use them, so it's really worth considering it.

Am Freitag, den 01.01.2010, 23:05 +0100 schrieb Samuel Thibault:

Quoted 6 lines

Quoted 2 lines · Stefani Seibold, le Fri 01 Jan 2010 16:10:16 +0100, a écrit :

But ps is not a performance critical application nor a cat /proc/*/status!

Errr, maybe not so much as some other operations, but a lot of tools use them, so it's really worth considering it.

Right. And i am still trying to find some optimization. Stay tuned.

2010/1/2 Stefani Seibold <[email protected]>:

Quoted 5 lines

Am Freitag, den 01.01.2010, 23:05 +0100 schrieb Samuel Thibault:

Quoted 4 lines

Quoted 2 lines · Stefani Seibold, le Fri 01 Jan 2010 16:10:16 +0100, a écrit :

But ps is not a performance critical application nor a cat /proc/*/status!

Quoted 4 lines

Quoted 2 lines

Errr, maybe not so much as some other operations, but a lot of tools use them, so it's really worth considering it.

Right. And i am still trying to find some optimization. Stay tuned.

I can understand you feel sad. but don't worry. reverting is very usual event on kernel development. You can retry anytime if you make optimized code.

As announce here is the patch to partial revert the show stack information patch due a not accepted performance regression. It will be now show only the current stack usage, not the high water mark.

The path is only partial reverted because i need the other parts to do it in an other way.

There are now two possibilities solutions:

- create a new /proc/<pid>/stackinfo entry, which provides the reverted information and maybe others like the sigaltstack.
- create a user space tool which use /proc/<pid>/pagemap

In both cases the information of task->stack_start and the KSTK_ESP is needed.

It will be also needed for an enhancement of the oom handler, where i free unused stack pages (the pages before the stack pointer) under high memory pressure. This is currently under work.

Andrew please apply this patch to 2.6.34-rc* tree.

Greetings,
Stefani

Signed-off-by Stefani Seibold <[email protected]>

---
 Documentation/filesystems/proc.txt |    2 
 fs/proc/array.c                    |   87 ++-----------------------------------
 2 files changed, 8 insertions(+), 81 deletions(-)

diff -u -N -r -p linux-2.6.33-rc2.orig/fs/proc/array.c linux-2.6.33-rc2.new/fs/proc/array.c

fs/proc/array.c +7 -80

@@ -327,93 +327,20 @@ static inline void task_context_switch_c
 			p->nivcsw);
 }
 
-#ifdef CONFIG_MMU
-
-struct stack_stats {
-	struct vm_area_struct *vma;
-	unsigned long	startpage;
-	unsigned long	usage;
-};
-
-static int stack_usage_pte_range(pmd_t *pmd, unsigned long addr,
-				unsigned long end, struct mm_walk *walk)
+static inline void task_show_stack_usage(struct seq_file *m,
+						struct task_struct *task)
 {
-	struct stack_stats *ss = walk->private;
-	struct vm_area_struct *vma = ss->vma;
-	pte_t *pte, ptent;
-	spinlock_t *ptl;
-	int ret = 0;
-
-	pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
-	for (; addr != end; pte++, addr += PAGE_SIZE) {
-		ptent = *pte;
+	unsigned long		usage;
 
+	if (task->mm) {
 #ifdef CONFIG_STACK_GROWSUP
-		if (pte_present(ptent) || is_swap_pte(ptent))
-			ss->usage = addr - ss->startpage + PAGE_SIZE;
+		usage = KSTK_ESP(task) - task->stack_start;
 #else
-		if (pte_present(ptent) || is_swap_pte(ptent)) {
-			ss->usage = ss->startpage - addr + PAGE_SIZE;
-			pte++;
-			ret = 1;
-			break;
-		}
+		usage = task->stack_start - KSTK_ESP(task);
 #endif
+		seq_printf(m, "Stack usage:\t%lu kB\n", (usage + 1023) >> 10);
 	}
-	pte_unmap_unlock(pte - 1, ptl);
-	cond_resched();
-	return ret;
-}
-
-static inline unsigned long get_stack_usage_in_bytes(struct vm_area_struct *vma,
-				struct task_struct *task)
-{
-	struct stack_stats ss;
-	struct mm_walk stack_walk = {
-		.pmd_entry = stack_usage_pte_range,
-		.mm = vma->vm_mm,
-		.private = &ss,
-	};
-
-	if (!vma->vm_mm || is_vm_hugetlb_page(vma))
-		return 0;
-
-	ss.vma = vma;
-	ss.startpage = task->stack_start & PAGE_MASK;
-	ss.usage = 0;
-
-#ifdef CONFIG_STACK_GROWSUP
-	walk_page_range(KSTK_ESP(task) & PAGE_MASK, vma->vm_end,
-		&stack_walk);
-#else
-	walk_page_range(vma->vm_start, (KSTK_ESP(task) & PAGE_MASK) + PAGE_SIZE,
-		&stack_walk);
-#endif
-	return ss.usage;
-}
-
-static inline void task_show_stack_usage(struct seq_file *m,
-						struct task_struct *task)
-{
-	struct vm_area_struct	*vma;
-	struct mm_struct	*mm = get_task_mm(task);
-
-	if (mm) {
-		down_read(&mm->mmap_sem);
-		vma = find_vma(mm, task->stack_start);
-		if (vma)
-			seq_printf(m, "Stack usage:\t%lu kB\n",
-				get_stack_usage_in_bytes(vma, task) >> 10);
-
-		up_read(&mm->mmap_sem);
-		mmput(mm);
-	}
-}
-#else
-static void task_show_stack_usage(struct seq_file *m, struct task_struct *task)
-{
 }
-#endif		/* CONFIG_MMU */
 
 static void task_cpus_allowed(struct seq_file *m, struct task_struct *task)
 {

diff -u -N -r -p linux-2.6.33-rc2.orig/Documentation/filesystems/proc.txt linux-2.6.33-rc2.new/Documentation/filesystems/proc.txt

Documentation/filesystems/proc.txt +1 -1

@@ -231,7 +231,7 @@ Table 1-2: Contents of the statm files (
  Mems_allowed_list           Same as previous, but in "list format"
  voluntary_ctxt_switches     number of voluntary context switches
  nonvoluntary_ctxt_switches  number of non voluntary context switches
- Stack usage:                stack usage high water mark (round up to page size)
+ Stack usage:                stack usage in kB
..............................................................................
 
 Table 1-3: Contents of the statm files (as of 2.6.8-rc3)

Quoted 2 lines · Stefani Seibold, le Fri 01 Jan 2010 16:10:16 +0100, a écrit :

But ps is not a performance critical application nor a cat /proc/*/status!

Errr, maybe not so much as some other operations, but a lot of tools use them, so it's really worth considering it.

Anyway, I revert the regresstion patch as other regression patches. if you really want this feature, you have three options.

1. create new /proc file instead to use /proc/pid/status.
2. improve performance until typical use-case don't notice regression.
3. change ps and other /proc related userland implementation and resubmit this patch.

But even if you choose anything, You have to test both its functional and performance _before_ submitting kernel patch.

Quoted 8 lines · KOSAKI Motohiro

Anyway, I revert the regresstion patch as other regression patches. if you really want this feature, you have three options.

1. create new /proc file instead to use /proc/pid/status.
2. improve performance until typical use-case don't notice regression.
3. change ps and other /proc related userland implementation and resubmit this patch.

Quoted 2 lines

But even if you choose anything, You have to test both its functional and performance _before_ submitting kernel patch.

4. Leave everything alone (revert all the commits) and use a ptrace based tool to get this information when you need it.

Thus, this patch revert it.

Quoted 1 line · On Thu, 31 Dec 2009 23:12:22 +0900 (JST) KOSAKI Motohiro…

Thus, this patch revert it.

OK, thanks, I applied the full revert.

We can look at a new and less costly implementation of this feature for 2.6.34 if desired. In which case, all we have done is to delay the feature by one release, which is not a big deal.

Quoted 10 lines

Quoted 1 line · On Thu, 31 Dec 2009 23:12:22 +0900 (JST) KOSAKI Motohiro…

Thus, this patch revert it.

OK, thanks, I applied the full revert.

We can look at a new and less costly implementation of this feature for 2.6.34 if desired. In which case, all we have done is to delay the feature by one release, which is not a big deal.

Thanks.

note: my patch isn't full revert too. Commit d899bf7b introduced two feature (btw, I don't like one patch have two feature).

1) Add the annotattion of [thread stack: xxxx] mark to

    /proc/{pid}/task/{tid}/maps.

2) Add StackUsage field to /proc/{pid}/status.

I only revert (2), because I haven't seen (1) cause regression.