Detailed Stack Information Patch [1/3]

Detailed Stack Information Patch [1/3]

[Stefani Seibold]

diff -u -N -r linux-2.6.28.1.orig/fs/exec.c linux-2.6.28.1/fs/exec.c
--- linux-2.6.28.1.orig/fs/exec.c	2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/fs/exec.c	2009-01-20 09:10:35.000000000 +0100
@@ -1342,6 +1342,9 @@
 	current->flags &= ~PF_KTHREAD;
 	retval = search_binary_handler(bprm,regs);
 	if (retval >= 0) {
+#ifdef CONFIG_PROC_STACK
+		current->stack_start = current->mm->start_stack;
+#endif
 		/* execve success */
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
diff -u -N -r linux-2.6.28.1.orig/fs/proc/array.c linux-2.6.28.1/fs/proc/array.c
--- linux-2.6.28.1.orig/fs/proc/array.c	2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/fs/proc/array.c	2009-01-20 09:10:35.000000000 +0100
@@ -308,6 +308,25 @@
 			p->nivcsw);
 }
 
+#ifdef CONFIG_PROC_STACK
+static inline void task_show_stack_usage(struct seq_file *m,
+						struct task_struct *p)
+{
+	unsigned long		cur_stack;
+	unsigned long		base_page;
+
+	base_page = KSTK_ESP(p) >> PAGE_SHIFT;
+
+#ifdef CONFIG_STACK_GROWSUP
+	cur_stack = base_page-(p->stack_start >> PAGE_SHIFT);
+#else
+	cur_stack = (p->stack_start >> PAGE_SHIFT)-base_page;
+#endif
+	seq_printf(m,	"stack usage:\t%lu kB\n",
+		(cur_stack + 1) << (PAGE_SHIFT-10));
+}
+#endif
+
 int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task)
 {
@@ -327,6 +346,9 @@
 	task_show_regs(m, task);
 #endif
 	task_context_switch_counts(m, task);
+#ifdef CONFIG_PROC_STACK
+	task_show_stack_usage(m, task);
+#endif
 	return 0;
 }
 
diff -u -N -r linux-2.6.28.1.orig/fs/proc/task_mmu.c linux-2.6.28.1/fs/proc/task_mmu.c
--- linux-2.6.28.1.orig/fs/proc/task_mmu.c	2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/fs/proc/task_mmu.c	2009-01-20 09:10:35.000000000 +0100
@@ -240,6 +240,11 @@
 				} else if (vma->vm_start <= mm->start_stack &&
 					   vma->vm_end >= mm->start_stack) {
 					name = "[stack]";
+#ifdef CONFIG_PROC_STACK
+				} else if (vma->vm_start <= mm->start_stack &&
+					   vma->vm_end >= mm->start_stack) {
+					name = "[thread stack]";
+#endif
 				}
 			} else {
 				name = "[vdso]";
diff -u -N -r linux-2.6.28.1.orig/include/linux/sched.h linux-2.6.28.1/include/linux/sched.h
--- linux-2.6.28.1.orig/include/linux/sched.h	2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/include/linux/sched.h	2009-01-20 09:10:35.000000000 +0100
@@ -1356,6 +1356,9 @@
 	unsigned long default_timer_slack_ns;
 
 	struct list_head	*scm_work_list;
+#ifdef CONFIG_PROC_STACK
+	unsigned long stack_start;
+#endif
 };
 
 /*
diff -u -N -r linux-2.6.28.1.orig/init/Kconfig linux-2.6.28.1/init/Kconfig
--- linux-2.6.28.1.orig/init/Kconfig	2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/init/Kconfig	2009-01-20 09:10:36.000000000 +0100
@@ -814,6 +814,18 @@
 
 source "arch/Kconfig"
 
+config PROC_STACK
+ 	default y
+	depends on PROC_FS && MMU
+	bool "Enable /proc/<pid> stack monitoring" if EMBEDDED
+ 	help
+	  This enables monitoring of process and thread stack utilization.
+
+	  The /proc/pid/maps, /proc/pid/smaps, /proc/pid/status and the
+	  /proc/pid/task/pid pedants will be extended by the stack information.
+	  Disabling these interfaces will reduce the size of the kernel by
+	  approximately 1kb.
+
 endmenu		# General setup
 
 config HAVE_GENERIC_DMA_COHERENT
diff -u -N -r linux-2.6.28.1.orig/kernel/fork.c linux-2.6.28.1/kernel/fork.c
--- linux-2.6.28.1.orig/kernel/fork.c	2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/kernel/fork.c	2009-01-20 09:10:36.000000000 +0100
@@ -1093,6 +1093,11 @@
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+#ifdef CONFIG_PROC_STACK
+	p->stack_start = (stack_start == KSTK_ESP(current)) ?
+		current->stack_start : stack_start;
+#endif
+
 	/* Perform scheduler related setup. Assign this task to a CPU. */
 	sched_fork(p, clone_flags);
 

Detailed Stack Information Patch [1/3]

[Stefani Seibold]

diff -u -N -r linux-2.6.29.orig/fs/exec.c linux-2.6.29/fs/exec.c
--- linux-2.6.29.orig/fs/exec.c	2009-03-24 00:12:14.000000000 +0100
+++ linux-2.6.29/fs/exec.c	2009-03-31 16:02:55.000000000 +0200
@@ -1336,6 +1336,10 @@
 	if (retval < 0)
 		goto out;
 
+#ifdef CONFIG_PROC_STACK
+	current->stack_start = current->mm->start_stack;
+#endif
+
 	/* execve succeeded */
 	mutex_unlock(&current->cred_exec_mutex);
 	acct_update_integrals(current);
diff -u -N -r linux-2.6.29.orig/fs/proc/array.c linux-2.6.29/fs/proc/array.c
--- linux-2.6.29.orig/fs/proc/array.c	2009-03-24 00:12:14.000000000 +0100
+++ linux-2.6.29/fs/proc/array.c	2009-03-31 16:00:19.000000000 +0200
@@ -320,6 +320,25 @@
 			p->nivcsw);
 }
 
+#ifdef CONFIG_PROC_STACK
+static inline void task_show_stack_usage(struct seq_file *m,
+						struct task_struct *p)
+{
+	unsigned long		cur_stack;
+	unsigned long		base_page;
+
+	base_page = KSTK_ESP(p) >> PAGE_SHIFT;
+
+#ifdef CONFIG_STACK_GROWSUP
+	cur_stack = base_page-(p->stack_start >> PAGE_SHIFT);
+#else
+	cur_stack = (p->stack_start >> PAGE_SHIFT)-base_page;
+#endif
+	seq_printf(m,	"stack usage:\t%lu kB\n",
+		(cur_stack + 1) << (PAGE_SHIFT-10));
+}
+#endif
+
 int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task)
 {
@@ -339,6 +358,9 @@
 	task_show_regs(m, task);
 #endif
 	task_context_switch_counts(m, task);
+#ifdef CONFIG_PROC_STACK
+	task_show_stack_usage(m, task);
+#endif
 	return 0;
 }
 
diff -u -N -r linux-2.6.29.orig/fs/proc/task_mmu.c linux-2.6.29/fs/proc/task_mmu.c
--- linux-2.6.29.orig/fs/proc/task_mmu.c	2009-03-24 00:12:14.000000000 +0100
+++ linux-2.6.29/fs/proc/task_mmu.c	2009-03-31 16:00:19.000000000 +0200
@@ -240,6 +240,18 @@
 				} else if (vma->vm_start <= mm->start_stack &&
 					   vma->vm_end >= mm->start_stack) {
 					name = "[stack]";
+#ifdef CONFIG_PROC_STACK
+				} else {
+					unsigned long stack_start;
+
+					stack_start =
+						((struct proc_maps_private *)
+						 m->private)->task->stack_start;
+
+					if (vma->vm_start <= stack_start && 
+					    vma->vm_end >= stack_start)
+						name="[thread stack]";
+#endif
 				}
 			} else {
 				name = "[vdso]";
diff -u -N -r linux-2.6.29.orig/include/linux/sched.h linux-2.6.29/include/linux/sched.h
--- linux-2.6.29.orig/include/linux/sched.h	2009-03-24 00:12:14.000000000 +0100
+++ linux-2.6.29/include/linux/sched.h	2009-03-31 16:00:45.000000000 +0200
@@ -1417,6 +1417,9 @@
 	/* state flags for use by tracers */
 	unsigned long trace;
 #endif
+#ifdef CONFIG_PROC_STACK
+	unsigned long stack_start;
+#endif
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */
diff -u -N -r linux-2.6.29.orig/init/Kconfig linux-2.6.29/init/Kconfig
--- linux-2.6.29.orig/init/Kconfig	2009-03-24 00:12:14.000000000 +0100
+++ linux-2.6.29/init/Kconfig	2009-03-31 16:00:19.000000000 +0200
@@ -952,6 +952,18 @@
 
 source "arch/Kconfig"
 
+config PROC_STACK
+ 	default y
+	depends on PROC_FS && MMU
+	bool "Enable /proc/<pid> stack monitoring" if EMBEDDED
+ 	help
+	  This enables monitoring of process and thread stack utilization.
+
+	  The /proc/pid/maps, /proc/pid/smaps, /proc/pid/status and the
+	  /proc/pid/task/pid pedants will be extended by the stack information.
+	  Disabling these interfaces will reduce the size of the kernel by
+	  approximately 1kb.
+
 endmenu		# General setup
 
 config HAVE_GENERIC_DMA_COHERENT
diff -u -N -r linux-2.6.29.orig/kernel/fork.c linux-2.6.29/kernel/fork.c
--- linux-2.6.29.orig/kernel/fork.c	2009-03-24 00:12:14.000000000 +0100
+++ linux-2.6.29/kernel/fork.c	2009-03-31 16:00:19.000000000 +0200
@@ -1098,6 +1098,11 @@
 	if (unlikely(current->ptrace))
 		ptrace_fork(p, clone_flags);
 
+#ifdef CONFIG_PROC_STACK
+	p->stack_start = (stack_start == KSTK_ESP(current)) ?
+		current->stack_start : stack_start;
+#endif
+
 	/* Perform scheduler related setup. Assign this task to a CPU. */
 	sched_fork(p, clone_flags);
 

Re: Detailed Stack Information Patch [1/3]

[Ingo Molnar]

* Stefani Seibold <stefani@seibold.net> wrote:

> diff -u -N -r linux-2.6.29.orig/fs/exec.c linux-2.6.29/fs/exec.c
> --- linux-2.6.29.orig/fs/exec.c	2009-03-24 00:12:14.000000000 +0100
> +++ linux-2.6.29/fs/exec.c	2009-03-31 16:02:55.000000000 +0200
> @@ -1336,6 +1336,10 @@
>  	if (retval < 0)
>  		goto out;
>  
> +#ifdef CONFIG_PROC_STACK
> +	current->stack_start = current->mm->start_stack;
> +#endif

Ok. The 1/3 patch, the whole "display where the stack is" thing is 
obviously useful and we know that.

Today we display this:

 earth4:~/tip> cat /proc/self/maps 
 00110000-00111000 r-xp 00110000 00:00 0          [vdso]
 0053e000-0055e000 r-xp 00000000 09:00 54591597   /lib/ld-2.9.so
 0055f000-00560000 r--p 00020000 09:00 54591597   /lib/ld-2.9.so
 00560000-00561000 rw-p 00021000 09:00 54591597   /lib/ld-2.9.so
 00563000-006d1000 r-xp 00000000 09:00 54591620   /lib/libc-2.9.so
 006d1000-006d3000 r--p 0016e000 09:00 54591620   /lib/libc-2.9.so
 006d3000-006d4000 rw-p 00170000 09:00 54591620   /lib/libc-2.9.so
 006d4000-006d7000 rw-p 006d4000 00:00 0 
 08048000-08054000 r-xp 00000000 09:00 27787363   /bin/cat
 08054000-08055000 rw-p 0000c000 09:00 27787363   /bin/cat
 09996000-099b7000 rw-p 09996000 00:00 0          [heap]
 b7db9000-b7fb9000 r--p 00000000 09:00 50364418   /usr/lib/locale/locale-archive
 b7fb9000-b7fbb000 rw-p b7fb9000 00:00 0 
 bffc7000-bffdc000 rw-p bffeb000 00:00 0          [stack]

I was the one who added the [stack], [heap] and [vdso] annotations a 
few years ago and user-space developers liked it very much.

Tools parsing these files wont break [they dont care about the final 
column] - so there's no ABI worries and we can certainly do more 
here and enhance it.

You extend the above output with (in essence):

> +#ifdef CONFIG_PROC_STACK
> +static inline void task_show_stack_usage(struct seq_file *m,
> +						struct task_struct *p)

It would be better to put this into a fresh, related feature that 
went upstream recently:

 spirit:~> cat /proc/self/stack
 [<ffffffff8101c333>] save_stack_trace_tsk+0x26/0x43
 [<ffffffff81129237>] proc_pid_stack+0x63/0xa1
 [<ffffffff8112a753>] proc_single_show+0x5c/0x79
 [<ffffffff810fb2d6>] seq_read+0x16f/0x34d
 [<ffffffff810e3eea>] vfs_read+0xab/0x108
 [<ffffffff810e4007>] sys_read+0x4a/0x6e
 [<ffffffff8101133a>] system_call_fastpath+0x16/0x1b
 [<ffffffffffffffff>] 0xffffffffffffffff

That displays the kernel stack data - and we could display 
information about the user-stack data as well.

This #ifdef:

> +#ifdef CONFIG_STACK_GROWSUP
> +	cur_stack = base_page-(p->stack_start >> PAGE_SHIFT);
> +#else
> +	cur_stack = (p->stack_start >> PAGE_SHIFT)-base_page;
> +#endif

Should be hidden in a task_user_stack() inline helper.

Another thing is:

> @@ -240,6 +240,18 @@
>  				} else if (vma->vm_start <= mm->start_stack &&
>  					   vma->vm_end >= mm->start_stack) {
>  					name = "[stack]";
> +#ifdef CONFIG_PROC_STACK
> +				} else {
> +					unsigned long stack_start;
> +
> +					stack_start =
> +						((struct proc_maps_private *)
> +						 m->private)->task->stack_start;
> +
> +					if (vma->vm_start <= stack_start && 
> +					    vma->vm_end >= stack_start)
> +						name="[thread stack]";
> +#endif

This too should be unconditional IMO (it's useful, and 
ultra-embedded systems worried about kernel .text size can turn off 
CONFIG_PROC_FS anyway), _and_ i think we could do even better.

How about extending /proc/X/maps with:

 b7db9000-b7fb9000 r--p 00000000 09:00 50364418   /usr/lib/locale/locale-archive
 b7fb9000-b7fbb000 rw-p b7fb9000 00:00 0 
 bffc7000-bffdc000 rw-p bffeb000 00:00 0          [stack, usage: 1391 kB]

This is deterministically parseable, and meaningful-at-a-glance. 
Similarly for 'thread stack'.

This way we dont need any new files in /proc - that just increases 
the per task memory overhead.

What do you think?

	Ingo

Re: Detailed Stack Information Patch [1/3]

[Stefani Seibold]

Am Mittwoch, den 01.04.2009, 21:31 +0200 schrieb Ingo Molnar:
> * Stefani Seibold <stefani@seibold.net> wrote:
> 
> > diff -u -N -r linux-2.6.29.orig/fs/exec.c linux-2.6.29/fs/exec.c
> > --- linux-2.6.29.orig/fs/exec.c	2009-03-24 00:12:14.000000000 +0100
> > +++ linux-2.6.29/fs/exec.c	2009-03-31 16:02:55.000000000 +0200
> > @@ -1336,6 +1336,10 @@
> >  	if (retval < 0)
> >  		goto out;
> >  
> > +#ifdef CONFIG_PROC_STACK
> > +	current->stack_start = current->mm->start_stack;
> > +#endif
> 
> Ok. The 1/3 patch, the whole "display where the stack is" thing is 
> obviously useful and we know that.
> 
> Today we display this:
> 
>  earth4:~/tip> cat /proc/self/maps 
>  00110000-00111000 r-xp 00110000 00:00 0          [vdso]
>  0053e000-0055e000 r-xp 00000000 09:00 54591597   /lib/ld-2.9.so
>  .
>  .
>  .
>  bffc7000-bffdc000 rw-p bffeb000 00:00 0          [stack]
> 
> I was the one who added the [stack], [heap] and [vdso] annotations a 
> few years ago and user-space developers liked it very much.
> 
> Tools parsing these files wont break [they dont care about the final 
> column] - so there's no ABI worries and we can certainly do more 
> here and enhance it.
> 
> You extend the above output with (in essence):
> 
> > +#ifdef CONFIG_PROC_STACK
> > +static inline void task_show_stack_usage(struct seq_file *m,
> > +						struct task_struct *p)
> 
> It would be better to put this into a fresh, related feature that 
> went upstream recently:
> 
>  spirit:~> cat /proc/self/stack
>  [<ffffffff8101c333>] save_stack_trace_tsk+0x26/0x43
>  .
>  .
>  .
> That displays the kernel stack data - and we could display 
> information about the user-stack data as well.
> 

/proc/self/stack is a good place for a more detailed information,
like the start address of the stack, the current usage and the highest
used address.

> This #ifdef:
> 
> > +#ifdef CONFIG_STACK_GROWSUP
> > +	cur_stack = base_page-(p->stack_start >> PAGE_SHIFT);
> > +#else
> > +	cur_stack = (p->stack_start >> PAGE_SHIFT)-base_page;
> > +#endif
> 
> Should be hidden in a task_user_stack() inline helper.
> 

Yes, this is more readable.

> Another thing is:
> 
> > @@ -240,6 +240,18 @@
> >  				} else if (vma->vm_start <= mm->start_stack &&
> >  					   vma->vm_end >= mm->start_stack) {
> >  					name = "[stack]";
> > +#ifdef CONFIG_PROC_STACK
> > +				} else {
> > +					unsigned long stack_start;
> > +
> > +					stack_start =
> > +						((struct proc_maps_private *)
> > +						 m->private)->task->stack_start;
> > +
> > +					if (vma->vm_start <= stack_start && 
> > +					    vma->vm_end >= stack_start)
> > +						name="[thread stack]";
> > +#endif
> 
> This too should be unconditional IMO (it's useful, and 
> ultra-embedded systems worried about kernel .text size can turn off 
> CONFIG_PROC_FS anyway), _and_ i think we could do even better.
> 

The CONFIG_PROC_STACK thing was only for test. I prefer it as an "always
on" feature.

> How about extending /proc/X/maps with:
> 
>  b7db9000-b7fb9000 r--p 00000000 09:00 50364418   /usr/lib/locale/locale-archive
>  b7fb9000-b7fbb000 rw-p b7fb9000 00:00 0 
>  bffc7000-bffdc000 rw-p bffeb000 00:00 0          [stack, usage: 1391 kB]
> 
> This is deterministically parseable, and meaningful-at-a-glance. 
> Similarly for 'thread stack'.
> 

Good idea. Should i write a new patch for this or will be this your job?

> This way we dont need any new files in /proc - that just increases 
> the per task memory overhead.
> 
> What do you think?
> 
> 	Ingo