Re: Detailed Stack Information Patch [1/3]

[Ingo Molnar <mingo@elte.hu>]

* 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