Messages in this thread | | | From | Max Filippov <> | Date | Tue, 12 Nov 2019 08:23:38 -0800 | Subject | Re: [PATCH v2] xtensa: improve stack dumping |
| |
On Tue, Nov 12, 2019 at 1:49 AM Petr Mladek <pmladek@suse.com> wrote: > > On Thu 2019-11-07 16:44:48, Max Filippov wrote: > > Calculate printable stack size and use print_hex_dump instead of > > opencoding it. > > Make size of stack dump configurable. > > Drop extra newline output in show_trace as its output format does not > > depend on CONFIG_KALLSYMS. > > > diff --git a/arch/xtensa/Kconfig.debug b/arch/xtensa/Kconfig.debug > > index 39de98e20018..83cc8d12fa0e 100644 > > --- a/arch/xtensa/Kconfig.debug > > +++ b/arch/xtensa/Kconfig.debug > > @@ -31,3 +31,10 @@ config S32C1I_SELFTEST > > It is easy to make wrong hardware configuration, this test should catch it early. > > > > Say 'N' on stable hardware. > > + > > +config PRINT_STACK_DEPTH > > + int "Stack depth to print" if DEBUG_KERNEL > > + default 64 > > + help > > + This option allows you to set the stack depth that the kernel > > + prints in stack traces. > > I would split this into separate patch.
Sure, I'll split it out.
> > diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c > > index 4a6c495ce9b6..fe090ab1cab8 100644 > > --- a/arch/xtensa/kernel/traps.c > > +++ b/arch/xtensa/kernel/traps.c > > @@ -491,32 +491,24 @@ void show_trace(struct task_struct *task, unsigned long *sp) > > > > pr_info("Call Trace:\n"); > > walk_stackframe(sp, show_trace_cb, NULL); > > -#ifndef CONFIG_KALLSYMS > > - pr_cont("\n"); > > -#endif > > } > > > > -static int kstack_depth_to_print = 24; > > +static int kstack_depth_to_print = CONFIG_PRINT_STACK_DEPTH; > > > > void show_stack(struct task_struct *task, unsigned long *sp) > > { > > - int i = 0; > > - unsigned long *stack; > > + size_t len; > > > > if (!sp) > > sp = stack_pointer(task); > > - stack = sp; > > > > - pr_info("Stack:\n"); > > + len = min((-(unsigned long)sp) & (THREAD_SIZE - 4), > > + kstack_depth_to_print * 4ul); > > I would replace the hardcoded 4 with sizeof(void *).
It's not necessarily pointers, more like register-wide entries on the stack. Ok, I guess I'll give it a name.
> > - for (i = 0; i < kstack_depth_to_print; i++) { > > - if (kstack_end(sp)) > > - break; > > - pr_cont(" %08lx", *sp++); > > - if (i % 8 == 7) > > - pr_cont("\n"); > > - } > > - show_trace(task, stack); > > + pr_info("Stack:\n"); > > + print_hex_dump(KERN_INFO, " ", DUMP_PREFIX_NONE, 32, 4, > > + sp, len, false); > > + show_trace(task, sp); > > } > > The conversion looks fine to me. It is up to you (as a maintainer) > what you do with the above proposal for two cosmetic changes ;-) > Either way, feel free to use: > > Reviewed-by: Petr Mladek <pmladek@suse.com>
Thanks for the review.
-- Max
| |