[PATCH] proc: remove /proc/*/status "Stack usage:"

[PATCH] proc: remove /proc/*/status "Stack usage:"

[Alexey Dobriyan]

We have /proc/*/pagemap to determine how much stack was consumed.
Leave "threadstack" though.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/array.c |   90 --------------------------------------------------------
 1 file changed, 90 deletions(-)

--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -82,7 +82,6 @@
 #include <linux/pid_namespace.h>
 #include <linux/ptrace.h>
 #include <linux/tracehook.h>
-#include <linux/swapops.h>
 
 #include <asm/pgtable.h>
 #include <asm/processor.h>
@@ -322,94 +321,6 @@ static inline void task_context_switch_counts(struct seq_file *m,
 			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)
-{
-	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;
-
-#ifdef CONFIG_STACK_GROWSUP
-		if (pte_present(ptent) || is_swap_pte(ptent))
-			ss->usage = addr - ss->startpage + PAGE_SIZE;
-#else
-		if (pte_present(ptent) || is_swap_pte(ptent)) {
-			ss->usage = ss->startpage - addr + PAGE_SIZE;
-			pte++;
-			ret = 1;
-			break;
-		}
-#endif
-	}
-	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 */
-
 int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task)
 {
@@ -429,7 +340,6 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 	task_show_regs(m, task);
 #endif
 	task_context_switch_counts(m, task);
-	task_show_stack_usage(m, task);
 	return 0;
 }
 

Re: [PATCH] proc: remove /proc/*/status "Stack usage:"

[Stefani Seibold]

Am Sonntag, den 01.11.2009, 16:15 +0300 schrieb Alexey Dobriyan:
> We have /proc/*/pagemap to determine how much stack was consumed.
> Leave "threadstack" though.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 

Stop! /proc/*/pagemap is not a real substitute for the stackm usage
patch. If you have problems with this patch i will have a look on it.

>  fs/proc/array.c |   90 --------------------------------------------------------
>  1 file changed, 90 deletions(-)
> 
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -82,7 +82,6 @@
>  #include <linux/pid_namespace.h>
>  #include <linux/ptrace.h>
>  #include <linux/tracehook.h>
> -#include <linux/swapops.h>
>  
>  #include <asm/pgtable.h>
>  #include <asm/processor.h>
> @@ -322,94 +321,6 @@ static inline void task_context_switch_counts(struct seq_file *m,
>  			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)
> -{
> -	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;
> -
> -#ifdef CONFIG_STACK_GROWSUP
> -		if (pte_present(ptent) || is_swap_pte(ptent))
> -			ss->usage = addr - ss->startpage + PAGE_SIZE;
> -#else
> -		if (pte_present(ptent) || is_swap_pte(ptent)) {
> -			ss->usage = ss->startpage - addr + PAGE_SIZE;
> -			pte++;
> -			ret = 1;
> -			break;
> -		}
> -#endif
> -	}
> -	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 */
> -
>  int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>  			struct pid *pid, struct task_struct *task)
>  {
> @@ -429,7 +340,6 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>  	task_show_regs(m, task);
>  #endif
>  	task_context_switch_counts(m, task);
> -	task_show_stack_usage(m, task);
>  	return 0;
>  }
>  

Re: [PATCH] proc: remove /proc/*/status "Stack usage:"

[Alexey Dobriyan]

On Sun, Nov 01, 2009 at 03:39:57PM +0100, Stefani Seibold wrote:
> Am Sonntag, den 01.11.2009, 16:15 +0300 schrieb Alexey Dobriyan:
> > We have /proc/*/pagemap to determine how much stack was consumed.
> > Leave "threadstack" though.
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> > 
> 
> Stop! /proc/*/pagemap is not a real substitute for the stackm usage
> patch. If you have problems with this patch i will have a look on it.

Why?

You grab thread stack start from /proc/*/stat, then for every page of
stack VMA, you lookup state of page, which is exactly what you're doing
by employing page walker.

Re: [PATCH] proc: remove /proc/*/status "Stack usage:"

[Stefani Seibold]

Am Sonntag, den 01.11.2009, 18:53 +0300 schrieb Alexey Dobriyan:
> On Sun, Nov 01, 2009 at 03:39:57PM +0100, Stefani Seibold wrote:
> > Am Sonntag, den 01.11.2009, 16:15 +0300 schrieb Alexey Dobriyan:
> > > We have /proc/*/pagemap to determine how much stack was consumed.
> > > Leave "threadstack" though.
> > > 
> > 
> > Stop! /proc/*/pagemap is not a real substitute for the stackm usage
> > patch. If you have problems with this patch i will have a look on it.
> 
> Why?
> 
> You grab thread stack start from /proc/*/stat, then for every page of
> stack VMA, you lookup state of page, which is exactly what you're doing
> by employing page walker.

First, as you figured out there is currently no way to get the stack
start of a thread. But i currently check this and hope to find a
solution in the next 24 hours.

Second, this patch aims to the embedded world and there is normally no
tools available when you need it, but /proc is always there. Especially
outside in the field.

Like Dr Frank. N. Furter in the "Rocky Horror Picture Show" says: remove
the cause but NOT the symptoms!

And the cause is the miss implemented KSTK_ESP in a x86_64. I also
checked all other architekture depended KSTK_ESP definitions and this
definitions looks okay for me. 

Stand by....

Re: [PATCH] proc: remove /proc/*/status "Stack usage:"

[Alexey Dobriyan]

On Mon, Nov 02, 2009 at 12:32:08AM +0100, Stefani Seibold wrote:
> Am Sonntag, den 01.11.2009, 18:53 +0300 schrieb Alexey Dobriyan:
> > On Sun, Nov 01, 2009 at 03:39:57PM +0100, Stefani Seibold wrote:
> > > Am Sonntag, den 01.11.2009, 16:15 +0300 schrieb Alexey Dobriyan:
> > > > We have /proc/*/pagemap to determine how much stack was consumed.
> > > > Leave "threadstack" though.
> > > > 
> > > 
> > > Stop! /proc/*/pagemap is not a real substitute for the stackm usage
> > > patch. If you have problems with this patch i will have a look on it.
> > 
> > Why?
> > 
> > You grab thread stack start from /proc/*/stat, then for every page of
> > stack VMA, you lookup state of page, which is exactly what you're doing
> > by employing page walker.
> 
> First, as you figured out there is currently no way to get the stack
> start of a thread. But i currently check this and hope to find a
> solution in the next 24 hours.
> 
> Second, this patch aims to the embedded world and there is normally no
> tools available when you need it, but /proc is always there. Especially
> outside in the field.

Poor embedded people!

There is even Documentation/vm/page-types.c which is trivially adapted
to show stack pages state.

> Like Dr Frank. N. Furter in the "Rocky Horror Picture Show" says: remove
> the cause but NOT the symptoms!

The root of the problem is that you're measuring badly defined value.

Kernel doesn't know SP register of a running task until it stops it.
Even if task is stopped or inside kernel, kernel doesn't know where
userpace stack starts, because, in theory, application can move it's
stack to anywhere.

> And the cause is the miss implemented KSTK_ESP in a x86_64. I also
> checked all other architekture depended KSTK_ESP definitions and this
> definitions looks okay for me. 

Re: [PATCH] proc: remove /proc/*/status "Stack usage:"

[Stefani Seibold]

> Poor embedded people!
> 

That's why embedded peoble don't like to contribute their work.

> There is even Documentation/vm/page-types.c which is trivially adapted
> to show stack pages state.
> 
> > Like Dr Frank. N. Furter in the "Rocky Horror Picture Show" says: remove
> > the cause but NOT the symptoms!
> 
> The root of the problem is that you're measuring badly defined value.
> 
> Kernel doesn't know SP register of a running task until it stops it.
> Even if task is stopped or inside kernel, kernel doesn't know where
> userpace stack starts, because, in theory, application can move it's
> stack to anywhere.
> 

It works fine for 99% percent of the use cases. Nothing is perfect,
especially in theory.

> > And the cause is the miss implemented KSTK_ESP in a x86_64. I also
> > checked all other architekture depended KSTK_ESP definitions and this
> > definitions looks okay for me. 

BTW: I had fixed the x86_65 KSTP_ESP implementation. I tested it with 32
bit and 64 bit user space application. I will send soon a patch. Thanks
for figure out the problem.

Stefani