| Message ID | 20211008111626.455137084@infradead.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | wchan: Fix wchan support | expand |
On Fri, Oct 08, 2021 at 01:15:34PM +0200, Peter Zijlstra wrote: > A few archs got save_stack_trace_tsk() vs in_sched_functions() wrong. As mentioned on the last patch, it's not clear to me what the intended semantic of save_stack_trace_tsk() is w.r.t. sched functions, as the naive reading is that it should report *everything* a task may return to. If it's meant to skip sched functions, I think we need some explicit documentation/commentary to that effect. In that case, there are other architectures that need a fixup (e.g. arm64). TBH, I don't think it *should* skip sched functions, and we should filter out sched functions as required at a higher level, or deprecate this interface in favour of arch_stack_walk() where it's easier to have common filter functions invoked during the walk.... Thanks, Mark. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/csky/kernel/stacktrace.c | 7 ++++++- > arch/mips/kernel/stacktrace.c | 27 ++++++++++++++++----------- > arch/nds32/kernel/stacktrace.c | 21 +++++++++++---------- > 3 files changed, 33 insertions(+), 22 deletions(-) > > --- a/arch/csky/kernel/stacktrace.c > +++ b/arch/csky/kernel/stacktrace.c > @@ -122,12 +122,17 @@ static bool save_trace(unsigned long pc, > return __save_trace(pc, arg, false); > } > > +static bool save_trace_nosched(unsigned long pc, void *arg) > +{ > + return __save_trace(pc, arg, true); > +} > + > /* > * Save stack-backtrace addresses into a stack_trace buffer. > */ > void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > { > - walk_stackframe(tsk, NULL, save_trace, trace); > + walk_stackframe(tsk, NULL, save_trace_nosched, trace); > } > EXPORT_SYMBOL_GPL(save_stack_trace_tsk); > > --- a/arch/mips/kernel/stacktrace.c > +++ b/arch/mips/kernel/stacktrace.c > @@ -66,16 +66,7 @@ static void save_context_stack(struct st > #endif > } > > -/* > - * Save stack-backtrace addresses into a stack_trace buffer. > - */ > -void save_stack_trace(struct stack_trace *trace) > -{ > - save_stack_trace_tsk(current, trace); > -} > -EXPORT_SYMBOL_GPL(save_stack_trace); > - > -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +static void __save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace, bool savesched) > { > struct pt_regs dummyregs; > struct pt_regs *regs = &dummyregs; > @@ -88,6 +79,20 @@ void save_stack_trace_tsk(struct task_st > regs->cp0_epc = tsk->thread.reg31; > } else > prepare_frametrace(regs); > - save_context_stack(trace, tsk, regs, tsk == current); > + save_context_stack(trace, tsk, regs, savesched); > +} > + > +/* > + * Save stack-backtrace addresses into a stack_trace buffer. > + */ > +void save_stack_trace(struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(current, trace, true); > +} > +EXPORT_SYMBOL_GPL(save_stack_trace); > + > +void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(tsk, trace, false); > } > EXPORT_SYMBOL_GPL(save_stack_trace_tsk); > --- a/arch/nds32/kernel/stacktrace.c > +++ b/arch/nds32/kernel/stacktrace.c > @@ -6,25 +6,16 @@ > #include <linux/stacktrace.h> > #include <linux/ftrace.h> > > -void save_stack_trace(struct stack_trace *trace) > -{ > - save_stack_trace_tsk(current, trace); > -} > -EXPORT_SYMBOL_GPL(save_stack_trace); > - > -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +static void __save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace, bool savesched) > { > unsigned long *fpn; > int skip = trace->skip; > - int savesched; > int graph_idx = 0; > > if (tsk == current) { > __asm__ __volatile__("\tori\t%0, $fp, #0\n":"=r"(fpn)); > - savesched = 1; > } else { > fpn = (unsigned long *)thread_saved_fp(tsk); > - savesched = 0; > } > > while (!kstack_end(fpn) && !((unsigned long)fpn & 0x3) > @@ -50,4 +41,14 @@ void save_stack_trace_tsk(struct task_st > fpn = (unsigned long *)fpp; > } > } > +void save_stack_trace(struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(current, trace, true); > +} > +EXPORT_SYMBOL_GPL(save_stack_trace); > + > +void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(tsk, trace, false); > +} > EXPORT_SYMBOL_GPL(save_stack_trace_tsk); > >
Thx Peter, On Fri, Oct 8, 2021 at 7:18 PM Peter Zijlstra <peterz@infradead.org> wrote: > > A few archs got save_stack_trace_tsk() vs in_sched_functions() wrong. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/csky/kernel/stacktrace.c | 7 ++++++- > arch/mips/kernel/stacktrace.c | 27 ++++++++++++++++----------- > arch/nds32/kernel/stacktrace.c | 21 +++++++++++---------- > 3 files changed, 33 insertions(+), 22 deletions(-) > > --- a/arch/csky/kernel/stacktrace.c > +++ b/arch/csky/kernel/stacktrace.c > @@ -122,12 +122,17 @@ static bool save_trace(unsigned long pc, > return __save_trace(pc, arg, false); > } > > +static bool save_trace_nosched(unsigned long pc, void *arg) > +{ > + return __save_trace(pc, arg, true); > +} > + > /* > * Save stack-backtrace addresses into a stack_trace buffer. > */ > void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > { > - walk_stackframe(tsk, NULL, save_trace, trace); > + walk_stackframe(tsk, NULL, save_trace_nosched, trace); > } > EXPORT_SYMBOL_GPL(save_stack_trace_tsk); I think the patch should be: @@ -138,7 +138,7 @@ static bool __save_trace(unsigned long pc, void *arg, bool nosched) static bool save_trace(unsigned long pc, void *arg) { - return __save_trace(pc, arg, false); + return __save_trace(pc, arg, true); } Another question: If we put sched_text in the backtrace buffer, just cause put no useful information in wchan, right? (I think it wouldn't cause a worse problem than debugging.) > > --- a/arch/mips/kernel/stacktrace.c > +++ b/arch/mips/kernel/stacktrace.c > @@ -66,16 +66,7 @@ static void save_context_stack(struct st > #endif > } > > -/* > - * Save stack-backtrace addresses into a stack_trace buffer. > - */ > -void save_stack_trace(struct stack_trace *trace) > -{ > - save_stack_trace_tsk(current, trace); > -} > -EXPORT_SYMBOL_GPL(save_stack_trace); > - > -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +static void __save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace, bool savesched) > { > struct pt_regs dummyregs; > struct pt_regs *regs = &dummyregs; > @@ -88,6 +79,20 @@ void save_stack_trace_tsk(struct task_st > regs->cp0_epc = tsk->thread.reg31; > } else > prepare_frametrace(regs); > - save_context_stack(trace, tsk, regs, tsk == current); > + save_context_stack(trace, tsk, regs, savesched); > +} > + > +/* > + * Save stack-backtrace addresses into a stack_trace buffer. > + */ > +void save_stack_trace(struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(current, trace, true); > +} > +EXPORT_SYMBOL_GPL(save_stack_trace); > + > +void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(tsk, trace, false); > } > EXPORT_SYMBOL_GPL(save_stack_trace_tsk); > --- a/arch/nds32/kernel/stacktrace.c > +++ b/arch/nds32/kernel/stacktrace.c > @@ -6,25 +6,16 @@ > #include <linux/stacktrace.h> > #include <linux/ftrace.h> > > -void save_stack_trace(struct stack_trace *trace) > -{ > - save_stack_trace_tsk(current, trace); > -} > -EXPORT_SYMBOL_GPL(save_stack_trace); > - > -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +static void __save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace, bool savesched) > { > unsigned long *fpn; > int skip = trace->skip; > - int savesched; > int graph_idx = 0; > > if (tsk == current) { > __asm__ __volatile__("\tori\t%0, $fp, #0\n":"=r"(fpn)); > - savesched = 1; > } else { > fpn = (unsigned long *)thread_saved_fp(tsk); > - savesched = 0; > } > > while (!kstack_end(fpn) && !((unsigned long)fpn & 0x3) > @@ -50,4 +41,14 @@ void save_stack_trace_tsk(struct task_st > fpn = (unsigned long *)fpp; > } > } > +void save_stack_trace(struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(current, trace, true); > +} > +EXPORT_SYMBOL_GPL(save_stack_trace); > + > +void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) > +{ > + __save_stack_trace_tsk(tsk, trace, false); > +} > EXPORT_SYMBOL_GPL(save_stack_trace_tsk); > > -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
--- a/arch/csky/kernel/stacktrace.c +++ b/arch/csky/kernel/stacktrace.c @@ -122,12 +122,17 @@ static bool save_trace(unsigned long pc, return __save_trace(pc, arg, false); } +static bool save_trace_nosched(unsigned long pc, void *arg) +{ + return __save_trace(pc, arg, true); +} + /* * Save stack-backtrace addresses into a stack_trace buffer. */ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) { - walk_stackframe(tsk, NULL, save_trace, trace); + walk_stackframe(tsk, NULL, save_trace_nosched, trace); } EXPORT_SYMBOL_GPL(save_stack_trace_tsk); --- a/arch/mips/kernel/stacktrace.c +++ b/arch/mips/kernel/stacktrace.c @@ -66,16 +66,7 @@ static void save_context_stack(struct st #endif } -/* - * Save stack-backtrace addresses into a stack_trace buffer. - */ -void save_stack_trace(struct stack_trace *trace) -{ - save_stack_trace_tsk(current, trace); -} -EXPORT_SYMBOL_GPL(save_stack_trace); - -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) +static void __save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace, bool savesched) { struct pt_regs dummyregs; struct pt_regs *regs = &dummyregs; @@ -88,6 +79,20 @@ void save_stack_trace_tsk(struct task_st regs->cp0_epc = tsk->thread.reg31; } else prepare_frametrace(regs); - save_context_stack(trace, tsk, regs, tsk == current); + save_context_stack(trace, tsk, regs, savesched); +} + +/* + * Save stack-backtrace addresses into a stack_trace buffer. + */ +void save_stack_trace(struct stack_trace *trace) +{ + __save_stack_trace_tsk(current, trace, true); +} +EXPORT_SYMBOL_GPL(save_stack_trace); + +void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) +{ + __save_stack_trace_tsk(tsk, trace, false); } EXPORT_SYMBOL_GPL(save_stack_trace_tsk); --- a/arch/nds32/kernel/stacktrace.c +++ b/arch/nds32/kernel/stacktrace.c @@ -6,25 +6,16 @@ #include <linux/stacktrace.h> #include <linux/ftrace.h> -void save_stack_trace(struct stack_trace *trace) -{ - save_stack_trace_tsk(current, trace); -} -EXPORT_SYMBOL_GPL(save_stack_trace); - -void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) +static void __save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace, bool savesched) { unsigned long *fpn; int skip = trace->skip; - int savesched; int graph_idx = 0; if (tsk == current) { __asm__ __volatile__("\tori\t%0, $fp, #0\n":"=r"(fpn)); - savesched = 1; } else { fpn = (unsigned long *)thread_saved_fp(tsk); - savesched = 0; } while (!kstack_end(fpn) && !((unsigned long)fpn & 0x3) @@ -50,4 +41,14 @@ void save_stack_trace_tsk(struct task_st fpn = (unsigned long *)fpp; } } +void save_stack_trace(struct stack_trace *trace) +{ + __save_stack_trace_tsk(current, trace, true); +} +EXPORT_SYMBOL_GPL(save_stack_trace); + +void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) +{ + __save_stack_trace_tsk(tsk, trace, false); +} EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
A few archs got save_stack_trace_tsk() vs in_sched_functions() wrong. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/csky/kernel/stacktrace.c | 7 ++++++- arch/mips/kernel/stacktrace.c | 27 ++++++++++++++++----------- arch/nds32/kernel/stacktrace.c | 21 +++++++++++---------- 3 files changed, 33 insertions(+), 22 deletions(-)