| Message ID | 6a8b68f8bd64f8c16d97ef943534c639781e7f77.1621577151.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | powerpc: Stack tracer fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (258eb1f3aaa9face35e613c229c1337263491ea0) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 2 warnings, 3 checks, 169 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Fri, 21 May 2021 12:18:36 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > In preparation to add support for stack tracer to powerpc, move code to > save stack trace and to calculate the frame sizes into a separate weak > function. Also provide access to some of the data structures used by the > stack trace code so that architectures can update those. > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > include/linux/ftrace.h | 8 ++++ > kernel/trace/trace_stack.c | 98 ++++++++++++++++++++------------------ > 2 files changed, 60 insertions(+), 46 deletions(-) > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index a69f363b61bf73..8263427379f05c 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -368,10 +368,18 @@ static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, > > #ifdef CONFIG_STACK_TRACER > > +#define STACK_TRACE_ENTRIES 500 > + > +extern unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; > +extern unsigned stack_trace_index[STACK_TRACE_ENTRIES]; > +extern unsigned int stack_trace_nr_entries; > +extern unsigned long stack_trace_max_size; > extern int stack_tracer_enabled; > > int stack_trace_sysctl(struct ctl_table *table, int write, void *buffer, > size_t *lenp, loff_t *ppos); > +void stack_get_trace(unsigned long traced_ip, unsigned long *stack_ref, > + unsigned long stack_size, int *tracer_frame); > > /* DO NOT MODIFY THIS VARIABLE DIRECTLY! */ > DECLARE_PER_CPU(int, disable_stack_tracer); > diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c > index 63c28504205162..5b63dbd37c8c25 100644 > --- a/kernel/trace/trace_stack.c > +++ b/kernel/trace/trace_stack.c > @@ -19,13 +19,11 @@ > > #include "trace.h" > > -#define STACK_TRACE_ENTRIES 500 > +unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; > +unsigned stack_trace_index[STACK_TRACE_ENTRIES]; > > -static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; > -static unsigned stack_trace_index[STACK_TRACE_ENTRIES]; > - > -static unsigned int stack_trace_nr_entries; > -static unsigned long stack_trace_max_size; > +unsigned int stack_trace_nr_entries; > +unsigned long stack_trace_max_size; > static arch_spinlock_t stack_trace_max_lock = > (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; > > @@ -152,49 +150,19 @@ static void print_max_stack(void) > * Although the entry function is not displayed, the first function (sys_foo) > * will still include the stack size of it. > */ > -static void check_stack(unsigned long ip, unsigned long *stack) I just got back from PTO and have a ton of other obligations to attend to before I can dig deeper into this. I'm not opposed to this change, but the stack_tracer has not been getting the love that it deserves and I think you hit one of the issues that needs to be addressed. I'm not sure this is a PPC only issue, and would like to see if I can get more time (or someone else can) to reevaluate the way stack tracer works, and see if it can be made a bit more robust. -- Steve
Steven Rostedt wrote: > On Fri, 21 May 2021 12:18:36 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> In preparation to add support for stack tracer to powerpc, move code to >> save stack trace and to calculate the frame sizes into a separate weak >> function. Also provide access to some of the data structures used by the >> stack trace code so that architectures can update those. >> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> include/linux/ftrace.h | 8 ++++ >> kernel/trace/trace_stack.c | 98 ++++++++++++++++++++------------------ >> 2 files changed, 60 insertions(+), 46 deletions(-) >> >> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h >> index a69f363b61bf73..8263427379f05c 100644 >> --- a/include/linux/ftrace.h >> +++ b/include/linux/ftrace.h >> @@ -368,10 +368,18 @@ static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, >> >> #ifdef CONFIG_STACK_TRACER >> >> +#define STACK_TRACE_ENTRIES 500 >> + >> +extern unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; >> +extern unsigned stack_trace_index[STACK_TRACE_ENTRIES]; >> +extern unsigned int stack_trace_nr_entries; >> +extern unsigned long stack_trace_max_size; >> extern int stack_tracer_enabled; >> >> int stack_trace_sysctl(struct ctl_table *table, int write, void *buffer, >> size_t *lenp, loff_t *ppos); >> +void stack_get_trace(unsigned long traced_ip, unsigned long *stack_ref, >> + unsigned long stack_size, int *tracer_frame); >> >> /* DO NOT MODIFY THIS VARIABLE DIRECTLY! */ >> DECLARE_PER_CPU(int, disable_stack_tracer); >> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c >> index 63c28504205162..5b63dbd37c8c25 100644 >> --- a/kernel/trace/trace_stack.c >> +++ b/kernel/trace/trace_stack.c >> @@ -19,13 +19,11 @@ >> >> #include "trace.h" >> >> -#define STACK_TRACE_ENTRIES 500 >> +unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; >> +unsigned stack_trace_index[STACK_TRACE_ENTRIES]; >> >> -static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; >> -static unsigned stack_trace_index[STACK_TRACE_ENTRIES]; >> - >> -static unsigned int stack_trace_nr_entries; >> -static unsigned long stack_trace_max_size; >> +unsigned int stack_trace_nr_entries; >> +unsigned long stack_trace_max_size; >> static arch_spinlock_t stack_trace_max_lock = >> (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; >> >> @@ -152,49 +150,19 @@ static void print_max_stack(void) >> * Although the entry function is not displayed, the first function (sys_foo) >> * will still include the stack size of it. >> */ >> -static void check_stack(unsigned long ip, unsigned long *stack) > > I just got back from PTO and have a ton of other obligations to attend > to before I can dig deeper into this. Thanks for the heads up. > I'm not opposed to this change, > but the stack_tracer has not been getting the love that it deserves and > I think you hit one of the issues that needs to be addressed. Sure. I started off by just updating arch_stack_walk() to return the traced function, but soon realized that the frame size determination can be made more robust on powerpc due to the ABI. > I'm not > sure this is a PPC only issue, and would like to see if I can get more > time (or someone else can) to reevaluate the way stack tracer works, > and see if it can be made a bit more robust. Sure. If you have specific issues to be looked at, please do elaborate. It seems to be working fine otherwise. The one limitation though is down to how ftrace works on powerpc -- the mcount call is before a function sets up its own stackframe. Due to this, we won't ever be able to account for the stackframe from a leaf function -- but, that's a fairly minor limitation. Thanks, Naveen
On Wed, 02 Jun 2021 16:05:18 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > It seems to be working fine otherwise. The one limitation though is down > to how ftrace works on powerpc -- the mcount call is before a function > sets up its own stackframe. Due to this, we won't ever be able to > account for the stackframe from a leaf function -- but, that's a fairly > minor limitation. And this is true for x86 as well because it no longer uses mcount, but uses fentry instead (called before stack setup), but I figured there's not much we could do about it. -- Steve
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index a69f363b61bf73..8263427379f05c 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -368,10 +368,18 @@ static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, #ifdef CONFIG_STACK_TRACER +#define STACK_TRACE_ENTRIES 500 + +extern unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; +extern unsigned stack_trace_index[STACK_TRACE_ENTRIES]; +extern unsigned int stack_trace_nr_entries; +extern unsigned long stack_trace_max_size; extern int stack_tracer_enabled; int stack_trace_sysctl(struct ctl_table *table, int write, void *buffer, size_t *lenp, loff_t *ppos); +void stack_get_trace(unsigned long traced_ip, unsigned long *stack_ref, + unsigned long stack_size, int *tracer_frame); /* DO NOT MODIFY THIS VARIABLE DIRECTLY! */ DECLARE_PER_CPU(int, disable_stack_tracer); diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c index 63c28504205162..5b63dbd37c8c25 100644 --- a/kernel/trace/trace_stack.c +++ b/kernel/trace/trace_stack.c @@ -19,13 +19,11 @@ #include "trace.h" -#define STACK_TRACE_ENTRIES 500 +unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; +unsigned stack_trace_index[STACK_TRACE_ENTRIES]; -static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES]; -static unsigned stack_trace_index[STACK_TRACE_ENTRIES]; - -static unsigned int stack_trace_nr_entries; -static unsigned long stack_trace_max_size; +unsigned int stack_trace_nr_entries; +unsigned long stack_trace_max_size; static arch_spinlock_t stack_trace_max_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; @@ -152,49 +150,19 @@ static void print_max_stack(void) * Although the entry function is not displayed, the first function (sys_foo) * will still include the stack size of it. */ -static void check_stack(unsigned long ip, unsigned long *stack) +void __weak stack_get_trace(unsigned long traced_ip, unsigned long *stack_ref, + unsigned long stack_size, int *tracer_frame) { - unsigned long this_size, flags; unsigned long *p, *top, *start; - static int tracer_frame; - int frame_size = READ_ONCE(tracer_frame); + unsigned long *p, *top, *start; int i, x; - this_size = ((unsigned long)stack) & (THREAD_SIZE-1); - this_size = THREAD_SIZE - this_size; - /* Remove the frame of the tracer */ - this_size -= frame_size; - - if (this_size <= stack_trace_max_size) - return; - - /* we do not handle interrupt stacks yet */ - if (!object_is_on_stack(stack)) - return; - - /* Can't do this from NMI context (can cause deadlocks) */ - if (in_nmi()) - return; - - local_irq_save(flags); - arch_spin_lock(&stack_trace_max_lock); - - /* In case another CPU set the tracer_frame on us */ - if (unlikely(!frame_size)) - this_size -= tracer_frame; - - /* a race could have already updated it */ - if (this_size <= stack_trace_max_size) - goto out; - - stack_trace_max_size = this_size; - stack_trace_nr_entries = stack_trace_save(stack_dump_trace, ARRAY_SIZE(stack_dump_trace) - 1, 0); /* Skip over the overhead of the stack tracer itself */ for (i = 0; i < stack_trace_nr_entries; i++) { - if (stack_dump_trace[i] == ip) + if (stack_dump_trace[i] == traced_ip) break; } @@ -209,7 +177,7 @@ static void check_stack(unsigned long ip, unsigned long *stack) * Now find where in the stack these are. */ x = 0; - start = stack; + start = stack_ref; top = (unsigned long *) (((unsigned long)start & ~(THREAD_SIZE-1)) + THREAD_SIZE); @@ -223,7 +191,7 @@ static void check_stack(unsigned long ip, unsigned long *stack) while (i < stack_trace_nr_entries) { int found = 0; - stack_trace_index[x] = this_size; + stack_trace_index[x] = stack_size; p = start; for (; p < top && i < stack_trace_nr_entries; p++) { @@ -233,7 +201,7 @@ static void check_stack(unsigned long ip, unsigned long *stack) */ if ((READ_ONCE_NOCHECK(*p)) == stack_dump_trace[i]) { stack_dump_trace[x] = stack_dump_trace[i++]; - this_size = stack_trace_index[x++] = + stack_size = stack_trace_index[x++] = (top - p) * sizeof(unsigned long); found = 1; /* Start the search from here */ @@ -245,10 +213,10 @@ static void check_stack(unsigned long ip, unsigned long *stack) * out what that is, then figure it out * now. */ - if (unlikely(!tracer_frame)) { - tracer_frame = (p - stack) * + if (unlikely(!*tracer_frame)) { + *tracer_frame = (p - stack_ref) * sizeof(unsigned long); - stack_trace_max_size -= tracer_frame; + stack_trace_max_size -= *tracer_frame; } } } @@ -272,6 +240,44 @@ static void check_stack(unsigned long ip, unsigned long *stack) #endif stack_trace_nr_entries = x; +} + +static void check_stack(unsigned long ip, unsigned long *stack) +{ + unsigned long this_size, flags; + static int tracer_frame; + int frame_size = READ_ONCE(tracer_frame); + + this_size = ((unsigned long)stack) & (THREAD_SIZE-1); + this_size = THREAD_SIZE - this_size; + /* Remove the frame of the tracer */ + this_size -= frame_size; + + if (this_size <= stack_trace_max_size) + return; + + /* we do not handle interrupt stacks yet */ + if (!object_is_on_stack(stack)) + return; + + /* Can't do this from NMI context (can cause deadlocks) */ + if (in_nmi()) + return; + + local_irq_save(flags); + arch_spin_lock(&stack_trace_max_lock); + + /* In case another CPU set the tracer_frame on us */ + if (unlikely(!frame_size)) + this_size -= tracer_frame; + + /* a race could have already updated it */ + if (this_size <= stack_trace_max_size) + goto out; + + stack_trace_max_size = this_size; + + stack_get_trace(ip, stack, this_size, &tracer_frame); if (task_stack_end_corrupted(current)) { print_max_stack();
In preparation to add support for stack tracer to powerpc, move code to save stack trace and to calculate the frame sizes into a separate weak function. Also provide access to some of the data structures used by the stack trace code so that architectures can update those. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- include/linux/ftrace.h | 8 ++++ kernel/trace/trace_stack.c | 98 ++++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 46 deletions(-)