Re: [patch] sched: fix single-depth wchan output

[Ingo Molnar <mingo@elte.hu>]

* Ken Chen <kenchen@google.com> wrote:

> On Wed, Nov 5, 2008 at 10:59 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > If it does stack walking manually then please update it to use
> > save_stack_trace() instead - that is the standard API that will
> > utilize the best possible stack walking machinery on the architecture
> > level.
> 
> OK, I pulled the patch out of our code that export stack trace via 
> /proc/pid/trace.  I didn't write this patch, but I think a better 
> choice would be to override struct stacktrace_ops print_trace_ops 
> with a memory buffer pointer to dump the stack into.  If you have 
> any comments, please let me know.  I will polish this patch up and 
> rebase to git-head.

hm, instead of modifying the lowlevel arch dump code, why not just use 
the existing save_stack_trace(), and render the output yourself via a 
trivial sprintf, just like kernel/lockdep.c does?

See kernel/stacktrace.c's print_stack_trace() - that could be extended 
with a sprintf_stack_trace() method. Allocate a large enough buffer 
dynamically, with a max of 128 stacktrace entries or so. (the output 
buffer is limited to 4K-ish anyway, right?)

As a bonus this will work on every architecture, not just x86.

a few other details:

> -	char namebuf[KSYM_NAME_LEN];
> +	char namebuf[128];

... time machine back to old crappy code ;-)

>  	symname = kallsyms_lookup(address, &symsize, &offset,
> -					&modname, namebuf);
> +				  &modname, namebuf);

ditto. But none of this has to be modified so you can just drop these 
bits.

> +	read_lock(&tasklist_lock);
> +	buf_show_task(posp, end, task);
> +	read_unlock(&tasklist_lock);

to get a stable trace we'll need more locking than that (tasklist_lock 
does not exclude scheduling, etc.) - but it takes care of the most 
important detail: tasks exiting from under us. So this should be OK.

>  #endif
> +	INF("trace",  S_IFREG|S_IRUGO, pid_trace),

that needs to be r-------- instead of r--r--r--, for security reasons.

	Ingo