Re: Detailed Stack Information Patch [1/3]

[Stefani Seibold <stefani@seibold.net>]

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