Re: [tip:x86/cleanups] x86, dumpstack: Use frame pointer during stack trace

[Frederic Weisbecker]

2011/3/23 Namhyung Kim <namhyung@xxxxxxxxx>:
> 2011-03-11 (ê), 00:02 +0100, Frederic Weisbecker:
>> On Thu, Mar 10, 2011 at 10:26:07PM +0000, tip-bot for Namhyung Kim wrote:
>> > Commit-ID: Â2f8058ae197236f9d5641850ce27f67d8f3e0b39
>> > Gitweb: Â Â http://git.kernel.org/tip/2f8058ae197236f9d5641850ce27f67d8f3e0b39
>> > Author: Â Â Namhyung Kim <namhyung@xxxxxxxxx>
>> > AuthorDate: Tue, 8 Mar 2011 20:44:22 +0900
>> > Committer: ÂThomas Gleixner <tglx@xxxxxxxxxxxxx>
>> > CommitDate: Thu, 10 Mar 2011 23:20:30 +0100
>> >
>> > x86, dumpstack: Use frame pointer during stack trace
>> >
>> > If CONFIG_FRAME_POINTER is set then use the frame pointer for the
>> > stack backtrace rather than scanning whole stack blindly.
>>
>> We don't do it blindly, we actually check the reliability with the
>> frame pointer.
>>
>> I'm not sure this patch is a good idea. stack dumps need to stay very
>> robust and not exclusively rely on the frame pointer to be correct.
>> At least walking blindly the stack provides a best effort dump as a last
>> resort.
>>
>
> Sounds reasonable. How about adding a boot param to control it then?

Hmm, but I'm not sure what it would be useful for. Even if one is sure that his
crash will have the needed reliable addresses already, having
unreliable ones too in
the report are not a problem. Are they?

Besides, how can we read a stack dump report posted by someone and be sure
he did not miss an important part because he had this kernel parameter enabled
and his frame pointer broken, or our frame based stack dump walking broken as
it has been many times, like your fixed in your patchset.

One win I could find though is that it can help a stack dump to fit in
the screen....

>
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 809d027de28f..0d7efd90c588 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -177,6 +177,16 @@ static const struct stacktrace_ops print_trace_ops
> #endif
> Â};
>
> +static int __init nofptrace_setup(char *s)
> +{
> + Â Â Â struct stacktrace_ops *ops;
> +
> + Â Â Â ops = (struct stacktrace_ops *) &print_trace_ops;
> + Â Â Â ops->walk_stack = walk_context_stack;
> + Â Â Â return 0;
> +}
> +early_param("nofptrace", nofptrace_setup);
> +
> Âvoid
> Âshow_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
> Â Â Â Â Â Â Â Âunsigned long *stack, unsigned long bp, char *log_lvl)
>
>
>
> --
> Regards,
> Namhyung Kim
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/