function_graph: Add separate depth counter to prevent trace corruption
Subsystems (17)
Steven Rostedt
maintainercover function_graph: Add separate depth counter to prevent trace corruption
While working on rewriting function graph tracer, I found a design flaw in the code. Commit 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback") tried to fix a bug that caused interrupts not to be traced if max_depth was set to 1 (for debugging NO_HZ_FULL), because if it came in just as another function that was being traced that had a depth of 1 was exiting. This caused the timing of being in the kernel to be off. The fix was simply to move the calling of the return function after the updating of curr_ret_stack index as that was what was being used.
The change log even says that it was safe to do this, but unfortunately it was not, and the barrier() there was specifically *for* that function (which shows why one should document barriers).
The problem is that the return callback may still be using what's on the shadow stack and by changing the shadow stack pointer, it may allow for another function being traced to overwrite that data. Note, if this happens, it will only cause garbage data to be traced and will not affect the actual operations of the kernel (ie. it wont crash).
Unfortunately just reverting that will bring back the old bug. The real way to fix that old bug is to create another counter to handle the depth, but this means that we need to change all the architectures that implement function graph tracing (that's 12 of them). Luckily, I need to do this anyway in my re-design so this is a good thing.
Since all the archictectures do basicall the same thing to prepare the function graph trace to be traced, I made a generic function that they all can use and simplified the logic of all the architectures. Then I'm able to fix the design problem in one place.
I pushed this code up to let zero-day have a whack at it, and I also downloaded the latest 8.1.0 cross compilers for all the archs that are affected and compiled tested them all (and got rid of all the warnings I introduced as well).
I marked this all for stable, but in reality it may not need to be ported as it will probably not be trivial to do so, becaues you'll need to also fix the architectures that are no longer used (although do we care about them?). But if someone really cares about correct timings of the function graph profiler when the options/graph-time is set to zero, then be my guest.
Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/urgent
Head SHA1: 1ffd62e282a649483afb0bb4d76f7244b3c10075
Steven Rostedt (VMware) (18):
function_graph: Create function_graph_enter() to consolidate architecture code x86/function_graph: Simplify with function_graph_entry()
ARM: function_graph: Simplify with function_graph_entry()
arm64: function_graph: Simplify with function_graph_entry()
microblaze: function_graph: Simplify with function_graph_entry()
MIPS: function_graph: Simplify with function_graph_entry()
nds32: function_graph: Simplify with function_graph_entry()
parisc: function_graph: Simplify with function_graph_entry() powerpc/function_graph: Simplify with function_graph_entry() riscv/function_graph: Simplify with function_graph_entry() s390/function_graph: Simplify with function_graph_entry() sh/function_graph: Simplify with function_graph_entry() sparc/function_graph: Simplify with function_graph_entry()
function_graph: Make ftrace_push_return_trace() static
function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
function_graph: Move return callback before update of curr_ret_stack
function_graph: Reverse the order of pushing the ret_stack and the callback
function_graph: Have profiler use curr_ret_stack and not depth
----
arch/arm/kernel/ftrace.c | 17 +------------ arch/arm64/kernel/ftrace.c | 15 +---------- arch/microblaze/kernel/ftrace.c | 15 ++--------- arch/mips/kernel/ftrace.c | 14 ++--------- arch/nds32/kernel/ftrace.c | 18 ++----------- arch/parisc/kernel/ftrace.c | 17 +++---------- arch/powerpc/kernel/trace/ftrace.c | 15 ++--------- arch/riscv/kernel/ftrace.c | 14 ++--------- arch/s390/kernel/ftrace.c | 13 ++-------- arch/sh/kernel/ftrace.c | 16 ++---------- arch/sparc/kernel/ftrace.c | 11 +------- arch/x86/kernel/ftrace.c | 15 +---------- include/linux/ftrace.h | 4 +-- include/linux/sched.h | 1 + kernel/trace/ftrace.c | 7 ++++-- kernel/trace/trace_functions_graph.c | 49 ++++++++++++++++++++++++++++-------- 16 files changed, 67 insertions(+), 174 deletions(-)
Steven Rostedt
maintainerPATCH 1/18 function_graph: Create function_graph_enter() to consolidate architecture code
From: "Steven Rostedt (VMware)" <[email protected]>
Currently all the architectures do basically the same thing in preparing the function graph tracer on entry to a function. This code can be pulled into a generic location and then this will allow the function graph tracer to be fixed, as well as extended.
Create a new function graph helper function_graph_enter() that will call the hook function (ftrace_graph_entry) and the shadow stack operation (ftrace_push_return_trace), and remove the need of the architecture code to manage the shadow stack.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- include/linux/ftrace.h | 3 +++ kernel/trace/trace_functions_graph.c | 16 ++++++++++++++++ 2 files changed, 19 insertions(+)
include/linux/ftrace.h +3 -0
@@ -779,6 +779,9 @@ extern void return_to_handler(void);extern int ftrace_push_return_trace (unsigned long ret,unsigned long func,int *depth,unsigned long frame_pointer,unsigned long *retp); +extern int +function_graph_enter (unsigned long ret,unsigned long func, +unsigned long frame_pointer,unsigned long *retp);unsigned long ftrace_graph_ret_addr (struct task_struct *task,int *idx,unsigned long ret,unsigned long *retp);
kernel/trace/trace_functions_graph.c +16 -0
@@ -182,6 +182,22 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,return 0 ; } +int function_graph_enter (unsigned long ret,unsigned long func, +unsigned long frame_pointer,unsigned long *retp) +{ +struct ftrace_graph_ent trace; + + trace.func = func; + trace.depth = current->curr_ret_stack +1 ; + +/* Only trace if the calling function expects to */ +if (!ftrace_graph_entry (&trace)) +return -EBUSY ; + +return ftrace_push_return_trace (ret, func, &trace.depth, + frame_pointer, retp); +} +/* Retrieve a function return address to the trace stack on thread info.*/ static void ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 2/18 x86/function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have x86 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/x86/kernel/ftrace.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
arch/x86/kernel/ftrace.c +1 -14
@@ -994,7 +994,6 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent, {unsigned long old;int faulted; -struct ftrace_graph_ent trace;unsigned long return_hooker = (unsigned long ) &return_to_handler;
@@ -1046,19 +1045,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent, return; } - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) { +if (function_graph_enter (old, self_addr, frame_pointer, parent)) *parent = old; -return ; - } - -if (ftrace_push_return_trace (old, self_addr, &trace.depth, - frame_pointer, parent) == -EBUSY ) { - *parent = old; -return ; - } }#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 3/18 ARM: function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have ARM use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/arm/kernel/ftrace.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-)
arch/arm/kernel/ftrace.c +1 -16
@@ -183,9 +183,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, unsigned long frame_pointer) {unsigned long return_hooker = (unsigned long ) &return_to_handler; -struct ftrace_graph_ent trace;unsigned long old; -int err;if (unlikely (atomic_read (¤t->tracing_graph_pause)))return ;
@@ -193,21 +191,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, old = *parent; *parent = return_hooker; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) { +if (function_graph_enter (old, self_addr, frame_pointer,NULL )) *parent = old; -return ; - } - - err =ftrace_push_return_trace (old, self_addr, &trace.depth, - frame_pointer,NULL ); -if (err == -EBUSY ) { - *parent = old; -return ; - } }#ifdef CONFIG_DYNAMIC_FTRACE
Signature · 1 line
2.19.1
Sasha Levin
maintainerPATCH 3/18 ARM: function_graph: Simplify with function_graph_entry()
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.
The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.
v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Build OK!
v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate
v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate
How should we proceed with this patch?
--
Thanks,
Sasha
Steven Rostedt
maintainerPATCH 4/18 arm64: function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have arm64 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/arm64/kernel/ftrace.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
arch/arm64/kernel/ftrace.c +1 -14
@@ -216,8 +216,6 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, {unsigned long return_hooker = (unsigned long )&return_to_handler;unsigned long old; -struct ftrace_graph_ent trace; -int err;if (unlikely (atomic_read (¤t->tracing_graph_pause)))return ;
@@ -229,18 +227,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,*/ old = *parent; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) -return ; - - err =ftrace_push_return_trace (old, self_addr, &trace.depth, - frame_pointer,NULL ); -if (err == -EBUSY ) -return ; -else +if (!function_graph_enter (old, self_addr, frame_pointer,NULL )) *parent = return_hooker; }
Signature · 1 line
2.19.1
Re: [for-next][PATCH 04/18] arm64: function_graph: Simplify with function_graph_entry()
Quoted 18 lines · Steven Rostedt
From: "Steven Rostedt (VMware)" <[email protected]>
The
function_graph_entry()function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependentprepare_ftrace_return().Have arm64 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the
curr_ret_stackis used.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")Signed-off-by Steven Rostedt (VMware) <[email protected]>
Quoted 3 lines
--- arch/arm64/kernel/ftrace.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
Minor nit: the subject refers to function_graph_entry(), but looks like you settled on function_graph_enter() in the end.
Anyway, looks like no change for us, so:
Will
Steven Rostedt
maintainerRe: [for-next][PATCH 04/18] arm64: function_graph: Simplify with function_graph_entry()
Quoted 23 lines · On Tue, 27 Nov 2018 18:07:23 +0000 Will Deacon <[email protected]> wrote:
Quoted 18 lines · Steven Rostedt
From: "Steven Rostedt (VMware)" <[email protected]>
The
function_graph_entry()function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependentprepare_ftrace_return().Have arm64 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the
curr_ret_stackis used.Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")Signed-off-by Steven Rostedt (VMware) <[email protected]>Quoted 3 lines
--- arch/arm64/kernel/ftrace.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
Quoted 2 lines
Minor nit: the subject refers to
function_graph_entry(), but looks like you settled onfunction_graph_enter()in the end.
Ah you're right!
Quoted 4 lines
Anyway, looks like no change for us, so:
Acked-by Will Deacon <[email protected]>
Thanks!
-- Steve
Steven Rostedt
maintainerPATCH 5/18 microblaze: function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have microblaze use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/microblaze/kernel/ftrace.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
arch/microblaze/kernel/ftrace.c +2 -13
@@ -22,8 +22,7 @@void prepare_ftrace_return (unsigned long *parent,unsigned long self_addr) {unsigned long old; -int faulted, err; -struct ftrace_graph_ent trace; +int faulted;unsigned long return_hooker = (unsigned long ) &return_to_handler;
@@ -63,18 +62,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr) return; } - err =ftrace_push_return_trace (old, self_addr, &trace.depth,0 ,NULL ); -if (err == -EBUSY ) { +if (function_graph_enter (old, self_addr,0 ,NULL )) *parent = old; -return ; - } - - trace.func = self_addr; -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) { - current->curr_ret_stack--; - *parent = old; - } }#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 6/18 MIPS: function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have MIPS use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/mips/kernel/ftrace.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
arch/mips/kernel/ftrace.c +2 -12
@@ -322,7 +322,6 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra, unsigned long fp) {unsigned long old_parent_ra; -struct ftrace_graph_ent trace;unsigned long return_hooker = (unsigned long ) &return_to_handler;int faulted, insns;
@@ -369,12 +368,6 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,if (unlikely (faulted)) goto out; -if (ftrace_push_return_trace (old_parent_ra, self_ra, &trace.depth, fp, -NULL ) == -EBUSY ) { - *parent_ra_addr = old_parent_ra; -return ; - } -/* * Get the recorded ip of the current mcount calling site in the * __mcount_loc section, which will be used to filter the function
@@ -382,13 +375,10 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,*/ insns =core_kernel_text (self_ra) ?2 :MCOUNT_OFFSET_INSNS +1 ; - trace.func = self_ra - (MCOUNT_INSN_SIZE * insns); + self_ra -= (MCOUNT_INSN_SIZE * insns); -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) { - current->curr_ret_stack--; +if (function_graph_enter (old_parent_ra, self_ra, fp,NULL )) *parent_ra_addr = old_parent_ra; - }return ; out:ftrace_graph_stop ();
Signature · 1 line
2.19.1
Sasha Levin
maintainerRe: [for-next][PATCH 06/18] MIPS: function_graph: Simplify with function_graph_entry()
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.
The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.
v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Failed to apply! Possible dependencies:
Unable to calculate
v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate
v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate
How should we proceed with this patch?
--
Thanks,
Sasha
Steven Rostedt
maintainerPATCH 7/18 nds32: function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have nds32 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/nds32/kernel/ftrace.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)
arch/nds32/kernel/ftrace.c +2 -16
@@ -211,29 +211,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, unsigned long frame_pointer) {unsigned long return_hooker = (unsigned long )&return_to_handler; -struct ftrace_graph_ent trace;unsigned long old; -int err;if (unlikely (atomic_read (¤t->tracing_graph_pause)))return ; old = *parent; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) -return ; - - err =ftrace_push_return_trace (old, self_addr, &trace.depth, - frame_pointer,NULL ); - -if (err == -EBUSY ) -return ; - - *parent = return_hooker; +if (!function_graph_enter (old, self_addr, frame_pointer,NULL )) + *parent = return_hooker; } noinline void ftrace_graph_caller(void)
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 8/18 parisc: function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have parisc use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/parisc/kernel/ftrace.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
arch/parisc/kernel/ftrace.c +3 -14
@@ -30,7 +30,6 @@ static void __hot prepare_ftrace_return(unsigned long *parent, unsigned long self_addr) {unsigned long old; -struct ftrace_graph_ent trace;extern int parisc_return_to_handler;if (unlikely (ftrace_graph_is_dead ()))
@@ -41,19 +40,9 @@ static void __hot prepare_ftrace_return(unsigned long *parent, old = *parent; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) -return ; - -if (ftrace_push_return_trace (old, self_addr, &trace.depth, -0 ,NULL ) == -EBUSY ) -return ; - -/* activate parisc_return_to_handler() as return point */ - *parent = (unsigned long) &parisc_return_to_handler; +if (!function_graph_enter (old, self_addr,0 ,NULL )) +/* activate parisc_return_to_handler() as return point */ + *parent = (unsigned long) &parisc_return_to_handler; }#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
2.19.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Helge Deller
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
HI Sasha,
Quoted 11 lines · Sasha Levin
This commit has been processed because it contains a "Fixes:" tag,
fixing commit:03274a3ffb44tracing/fgraph: Adjust fgraph depth before calling trace return callback.The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.
v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Build OK!
v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate
Quoted 4 lines
v3.18.126: Failed to apply! Possible dependencies:
Unable to calculateHow should we proceed with this patch?
My suggestion, although I didn't looked too much on it:
Apply it to v4.9 and higher only.
I think I started fixing trace functionality on parisc around 4.6, which is probably why applying it fails on v4.4 and v3.x
Helge
Steven Rostedt
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
Quoted 6 lines · On Fri, 23 Nov 2018 10:06:05 +0100 Helge Deller <[email protected]> wrote:
Quoted 1 line
How should we proceed with this patch?
My suggestion, although I didn't looked too much on it:
Apply it to v4.9 and higher only.
I think I started fixing trace functionality on parisc around 4.6, which is probably why applying it fails on v4.4 and v3.x
The problem is, if you backport the generic patches, it will completely break any arch that isn't updated. This also includes the archs that are no longer supported upstream, as they were not changed to handle the generic updates either.
-- Steve
Sasha Levin
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
Quoted 11 lines · Steven Rostedt
Quoted 6 lines · On Fri, 23 Nov 2018 10:06:05 +0100 Helge Deller <[email protected]> wrote:
Quoted 1 line
How should we proceed with this patch?
My suggestion, although I didn't looked too much on it:
Apply it to v4.9 and higher only.
I think I started fixing trace functionality on parisc around 4.6, which is probably why applying it fails on v4.4 and v3.x
Quoted 4 lines
The problem is, if you backport the generic patches, it will completely break any arch that isn't updated. This also includes the archs that are no longer supported upstream, as they were not changed to handle the generic updates either.
Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.
--
Thanks,
Sasha
Steven Rostedt
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
Quoted 3 lines · On Fri, 23 Nov 2018 13:34:15 -0500 Sasha Levin <[email protected]> wrote:
Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.
Nope. I stated that in my 0 patch.
-- Steve
Sasha Levin
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
Quoted 8 lines · Steven Rostedt
Quoted 3 lines · On Fri, 23 Nov 2018 13:34:15 -0500 Sasha Levin <[email protected]> wrote:
Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.
Nope. I stated that in my 0 patch.
That's not good though, if you don't intend for them to be automagically backported to stable trees by Greg, then they shouldn't be tagged at all and if someone is interested then he can provide a backport.
What will happen with these is that once Greg's scripts process Linus's tree he'll end up with this patch series inconsistently backported to stable trees, which is not what you want here.
Sure, we can wait for the "added to the xyz stable tree" mails and object then, but why risk breaking the trees?
--
Thanks.
Sasha
Steven Rostedt
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
Quoted 8 lines · On Fri, 23 Nov 2018 15:00:11 -0500 Sasha Levin <[email protected]> wrote:
Quoted 7 lines · Steven Rostedt
Quoted 3 lines · On Fri, 23 Nov 2018 13:34:15 -0500 Sasha Levin <[email protected]> wrote:
Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.
Quoted 5 lines
Quoted 1 line
Nope. I stated that in my 0 patch.
That's not good though, if you don't intend for them to be automagically backported to stable trees by Greg, then they shouldn't be tagged at all and if someone is interested then he can provide a backport.
For the most part they will be fine going back a few releases. But how far back is questionable before they start getting into issues. I talked a bit about this to Greg on IRC and he seemed fine with me adding the stable tag.
If they don't port back properly, it wont be a silent failure. They will either build or they wont. I'm suspect that you build all supported archs for your stable trees, right? If the patch fails to build, then either have someone that cares for that arch back port it, or don't back port the series. Simple as that.
Quoted 4 lines
What will happen with these is that once Greg's scripts process Linus's tree he'll end up with this patch series inconsistently backported to stable trees, which is not what you want here.
It's not like it won't work and then start to work again. Once they start failing in older versions, they will probably fail in all versions before that.
Quoted 3 lines
Sure, we can wait for the "added to the xyz stable tree" mails and object then, but why risk breaking the trees?
Again, it's not much different than other stable patches that need to be fixed for older trees. If they build, they are fine, if they don't then they need to be fixed. You'll know right at build time.
-- Steve
Sasha Levin
maintainerRe: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()
Quoted 11 lines · Steven Rostedt
Quoted 3 lines · On Fri, 23 Nov 2018 15:00:11 -0500 Sasha Levin <[email protected]> wrote:
What will happen with these is that once Greg's scripts process Linus's tree he'll end up with this patch series inconsistently backported to stable trees, which is not what you want here.
It's not like it won't work and then start to work again. Once they start failing in older versions, they will probably fail in all versions before that.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
Sure, we can wait for the "added to the xyz stable tree" mails and object then, but why risk breaking the trees?
Again, it's not much different than other stable patches that need to be fixed for older trees. If they build, they are fine, if they don't then they need to be fixed. You'll know right at build time.
So the only tree it applied and built was 4.19, all older trees failed on at least one arch.
I'm going to remove any parts of this series from all other trees.
--
Thanks,
Sasha
Steven Rostedt
maintainerPATCH 9/18 powerpc/function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have powerpc use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/powerpc/kernel/trace/ftrace.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
arch/powerpc/kernel/trace/ftrace.c +2 -13
@@ -950,7 +950,6 @@ int ftrace_disable_ftrace_graph_caller(void)*/ unsigned long prepare_ftrace_return (unsigned long parent,unsigned long ip) { -struct ftrace_graph_ent trace;unsigned long return_hooker;if (unlikely (ftrace_graph_is_dead ()))
@@ -961,18 +960,8 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip) return_hooker =ppc_function_entry (return_to_handler); - trace.func = ip; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) - goto out; - -if (ftrace_push_return_trace (parent, ip, &trace.depth,0 , -NULL ) == -EBUSY ) - goto out; - - parent = return_hooker; +if (!function_graph_enter (parent, ip,0 ,NULL )) + parent = return_hooker; out:return parent; }
Signature · 1 line
2.19.1
Sasha Levin
maintainerRe: [for-next][PATCH 09/18] powerpc/function_graph: Simplify with function_graph_entry()
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.
The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.
v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Failed to apply! Possible dependencies:
Unable to calculate
v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate
v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate
How should we proceed with this patch?
--
Thanks,
Sasha
Steven Rostedt
maintainerPATCH 10/18 riscv/function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have riscv use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/riscv/kernel/ftrace.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
arch/riscv/kernel/ftrace.c +2 -12
@@ -132,7 +132,6 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, {unsigned long return_hooker = (unsigned long )&return_to_handler;unsigned long old; -struct ftrace_graph_ent trace;int err;if (unlikely (atomic_read (¤t->tracing_graph_pause)))
@@ -144,17 +143,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,*/ old = *parent; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -if (!ftrace_graph_entry (&trace)) -return ; - - err =ftrace_push_return_trace (old, self_addr, &trace.depth, - frame_pointer, parent); -if (err == -EBUSY ) -return ; - *parent = return_hooker; +if (function_graph_enter (old, self_addr, frame_pointer, parent)) + *parent = return_hooker; }#ifdef CONFIG_DYNAMIC_FTRACE
Signature · 1 line
2.19.1
Re: [for-next][PATCH 10/18] riscv/function_graph: Simplify with function_graph_entry()
Quoted 44 lines · On Wed, 21 Nov 2018 16:28:11 PST (-0800), [email protected] wrote:
From: "Steven Rostedt (VMware)" <[email protected]>
The
function_graph_entry()function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependentprepare_ftrace_return().Have riscv use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the
curr_ret_stackis used.Cc Greentime Hu <[email protected]>Cc Alan Kao <[email protected]>Cc Palmer Dabbelt <[email protected]>Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")Signed-off-by Steven Rostedt (VMware) <[email protected]>--- arch/riscv/kernel/ftrace.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)arch/riscv/kernel/ftrace.c +0
-7@@ -132,7 +132,6 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, {unsigned long return_hooker = (unsigned long )&return_to_handler;unsigned long old; -struct ftrace_graph_ent trace;int err;if (unlikely (atomic_read (¤t->tracing_graph_pause)))@@ -144,17 +143,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,*/ old = *parent; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -if (!ftrace_graph_entry (&trace)) -return ; -
arch/riscv/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, · line 150
Quoted 10 lines
arch/riscv/kernel/ftrace.c +2
-5- err =ftrace_push_return_trace (old, self_addr, &trace.depth, - frame_pointer, parent); -if (err == -EBUSY ) -return ; - *parent = return_hooker; +if (function_graph_enter (old, self_addr, frame_pointer, parent)) + *parent = return_hooker; }#ifdef CONFIG_DYNAMIC_FTRACE
Steven Rostedt
maintainerPATCH 11/18 s390/function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have s390 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/s390/kernel/ftrace.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
arch/s390/kernel/ftrace.c +2 -11
@@ -203,22 +203,13 @@ device_initcall(ftrace_plt_init);*/ unsigned long prepare_ftrace_return (unsigned long parent,unsigned long ip) { -struct ftrace_graph_ent trace; -if (unlikely (ftrace_graph_is_dead ())) goto out;if (unlikely (atomic_read (¤t->tracing_graph_pause))) goto out; ip -=MCOUNT_INSN_SIZE ; - trace.func = ip; - trace.depth = current->curr_ret_stack +1 ; -/* Only trace if the calling function expects to. */ -if (!ftrace_graph_entry (&trace)) - goto out; -if (ftrace_push_return_trace (parent, ip, &trace.depth,0 , -NULL ) == -EBUSY ) - goto out; - parent = (unsigned long ) return_to_handler; +if (!function_graph_enter (parent, ip,0 ,NULL )) + parent = (unsigned long ) return_to_handler; out:return parent; }
Signature · 1 line
2.19.1
Re: [for-next][PATCH 11/18] s390/function_graph: Simplify with function_graph_entry()
Quoted 12 lines · On Wed, 21 Nov 2018 19:28:12 -0500 Steven Rostedt <[email protected]> wrote:
From: "Steven Rostedt (VMware)" <[email protected]>
The
function_graph_entry()function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependentprepare_ftrace_return().Have s390 use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the
curr_ret_stackis used.
Quoted 7 lines
Cc Martin Schwidefsky <[email protected]>Cc Heiko Carstens <[email protected]>Cc Julian Wiedmann <[email protected]>Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")Signed-off-by Steven Rostedt (VMware) <[email protected]>
A quick test showed that the patch series works fine on s390.
Quoted 33 lines
--- arch/s390/kernel/ftrace.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)arch/s390/kernel/ftrace.c +2
-11@@ -203,22 +203,13 @@ device_initcall(ftrace_plt_init);*/ unsigned long prepare_ftrace_return (unsigned long parent,unsigned long ip) { -struct ftrace_graph_ent trace; -if (unlikely (ftrace_graph_is_dead ())) goto out;if (unlikely (atomic_read (¤t->tracing_graph_pause))) goto out; ip -=MCOUNT_INSN_SIZE ; - trace.func = ip; - trace.depth = current->curr_ret_stack +1 ; -/* Only trace if the calling function expects to. */ -if (!ftrace_graph_entry (&trace)) - goto out; -if (ftrace_push_return_trace (parent, ip, &trace.depth,0 , -NULL ) == -EBUSY ) - goto out; - parent = (unsigned long ) return_to_handler; +if (!function_graph_enter (parent, ip,0 ,NULL )) + parent = (unsigned long ) return_to_handler; out:return parent; }
Signature · 4 lines
blue skies, Martin. "Reality continues to ruin my life." - Calvin.
Steven Rostedt
maintainerRe: [for-next][PATCH 11/18] s390/function_graph: Simplify with function_graph_entry()
Quoted 2 lines · On Thu, 22 Nov 2018 07:43:19 +0100 Martin Schwidefsky <[email protected]>…
A quick test showed that the patch series works fine on s390.
Thanks for testing, and the ack!
-- Steve
Steven Rostedt
maintainerPATCH 12/18 sh/function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have superh use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/sh/kernel/ftrace.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)
arch/sh/kernel/ftrace.c +2 -14
@@ -321,8 +321,7 @@ int ftrace_disable_ftrace_graph_caller(void)void prepare_ftrace_return (unsigned long *parent,unsigned long self_addr) {unsigned long old; -int faulted, err; -struct ftrace_graph_ent trace; +int faulted;unsigned long return_hooker = (unsigned long )&return_to_handler;if (unlikely (ftrace_graph_is_dead ()))
@@ -365,18 +364,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr) return; } - err =ftrace_push_return_trace (old, self_addr, &trace.depth,0 ,NULL ); -if (err == -EBUSY ) { +if (function_graph_enter (old, self_addr,0 ,NULL ))__raw_writel (old, parent); -return ; - } - - trace.func = self_addr; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) { - current->curr_ret_stack--; -__raw_writel (old, parent); - } }#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 13/18 sparc/function_graph: Simplify with function_graph_entry()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().
Have sparc use the new code, and remove the shadow stack management as well as having to set up the trace structure.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- arch/sparc/kernel/ftrace.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
arch/sparc/kernel/ftrace.c +1 -10
@@ -126,20 +126,11 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long frame_pointer) {unsigned long return_hooker = (unsigned long ) &return_to_handler; -struct ftrace_graph_ent trace;if (unlikely (atomic_read (¤t->tracing_graph_pause)))return parent +8UL ; - trace.func = self_addr; - trace.depth = current->curr_ret_stack +1 ; - -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) -return parent +8UL ; - -if (ftrace_push_return_trace (parent, self_addr, &trace.depth, - frame_pointer,NULL ) == -EBUSY ) +if (function_graph_enter (parent, self_addr, frame_pointer,NULL ))return parent +8UL ;return return_hooker;
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 14/18 function_graph: Make ftrace_push_return_trace() static
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
As all architectures now call function_graph_enter() to do the entry work, no architecture should ever call ftrace_push_return_trace(). Make it static.
This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.
--- include/linux/ftrace.h | 3 --- kernel/trace/trace_functions_graph.c | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-)
include/linux/ftrace.h +0 -3
@@ -776,9 +776,6 @@ struct ftrace_ret_stack {*/ extern void return_to_handler (void ); -extern int -ftrace_push_return_trace (unsigned long ret,unsigned long func,int *depth, -unsigned long frame_pointer,unsigned long *retp);extern int function_graph_enter (unsigned long ret,unsigned long func,unsigned long frame_pointer,unsigned long *retp);
kernel/trace/trace_functions_graph.c +1 -1
@@ -118,7 +118,7 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration, struct trace_seq *s, u32 flags);/* Add a function return address to the trace stack on thread info.*/ -int +static int ftrace_push_return_trace (unsigned long ret,unsigned long func,int *depth,unsigned long frame_pointer,unsigned long *retp) {
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 15/18 function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
Currently, the depth of the ret_stack is determined by curr_ret_stack index. The issue is that there's a race between setting of the curr_ret_stack and calling of the callback attached to the return of the function.
Commit 03274a3ffb44 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback") moved the calling of the callback to after the setting of the curr_ret_stack, even stating that it was safe to do so, when in fact, it was the reason there was a barrier() there (yes, I should have commented that barrier()).
Not only does the curr_ret_stack keep track of the current call graph depth, it also keeps the ret_stack content from being overwritten by new data.
The function profiler, uses the "subtime" variable of ret_stack structure and by moving the curr_ret_stack, it allows for interrupts to use the same structure it was using, corrupting the data, and breaking the profiler.
To fix this, there needs to be two variables to handle the call stack depth and the pointer to where the ret_stack is being used, as they need to change at two different locations.
--- include/linux/sched.h | 1 + kernel/trace/ftrace.c | 3 +++ kernel/trace/trace_functions_graph.c | 21 +++++++++++++-------- 3 files changed, 17 insertions(+), 8 deletions(-)
include/linux/sched.h +1 -0
@@ -1116,6 +1116,7 @@ struct task_struct {#ifdef CONFIG_FUNCTION_GRAPH_TRACER /* Index of current stored address in ret_stack: */ int curr_ret_stack; +int curr_ret_depth;/* Stack of return addresses for return function tracing: */ struct ftrace_ret_stack *ret_stack;
kernel/trace/ftrace.c +3 -0
@@ -6814,6 +6814,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)atomic_set (&t->tracing_graph_pause,0 );atomic_set (&t->trace_overrun,0 ); t->curr_ret_stack =-1 ; + t->curr_ret_depth =-1 ;/* Make sure the tasks see the -1 first: */ smp_wmb (); t->ret_stack = ret_stack_list[start++];
@@ -7038,6 +7039,7 @@ graph_init_task(struct task_struct *t, struct ftrace_ret_stack *ret_stack)void ftrace_graph_init_idle_task (struct task_struct *t,int cpu) { t->curr_ret_stack =-1 ; + t->curr_ret_depth =-1 ;/* * The idle task has no parent, it either has its own * stack or no stack at all.
@@ -7068,6 +7070,7 @@ void ftrace_graph_init_task(struct task_struct *t)/* Make sure we do not use the parent ret_stack */ t->ret_stack =NULL ; t->curr_ret_stack =-1 ; + t->curr_ret_depth =-1 ;if (ftrace_graph_active) {struct ftrace_ret_stack *ret_stack;
kernel/trace/trace_functions_graph.c +13 -8
@@ -119,7 +119,7 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,/* Add a function return address to the trace stack on thread info.*/ static int -ftrace_push_return_trace (unsigned long ret,unsigned long func,int *depth, +ftrace_push_return_trace (unsigned long ret,unsigned long func,unsigned long frame_pointer,unsigned long *retp) {unsigned long long calltime;
@@ -177,8 +177,6 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,#ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR current->ret_stack[index].retp = retp;#endif - *depth = current->curr_ret_stack; -return 0 ; }
@@ -188,14 +186,20 @@ int function_graph_enter(unsigned long ret, unsigned long func,struct ftrace_graph_ent trace; trace.func = func; - trace.depth = current->curr_ret_stack +1 ; + trace.depth = ++current->curr_ret_depth;/* Only trace if the calling function expects to */ if (!ftrace_graph_entry (&trace)) -return -EBUSY ; + goto out; -return ftrace_push_return_trace (ret, func, &trace.depth, - frame_pointer, retp); +if (ftrace_push_return_trace (ret, func, + frame_pointer, retp)) + goto out; + +return 0 ; + out: + current->curr_ret_depth--; +return -EBUSY ; }/* Retrieve a function return address to the trace stack on thread info.*/
@@ -257,7 +261,7 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret, trace->func = current->ret_stack[index].func; trace->calltime = current->ret_stack[index].calltime; trace->overrun =atomic_read (¤t->trace_overrun); - trace->depth = index; + trace->depth = current->curr_ret_depth; }/*
@@ -273,6 +277,7 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer) trace.rettime =trace_clock_local ();barrier (); current->curr_ret_stack--; + current->curr_ret_depth--;/* * The curr_ret_stack can be less than -1 only if it was * filtered out and it's about to return from the function.
Signature · 1 line
2.19.1
Peter Zijlstra
maintainerRe: [for-next][PATCH 15/18] function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
Quoted 6 lines · Steven Rostedt
kernel/trace/trace_functions_graph.c +1
-1@@ -188,14 +186,20 @@ int function_graph_enter(unsigned long ret, unsigned long func,struct ftrace_graph_ent trace; trace.func = func; - trace.depth = current->curr_ret_stack +1 ; + trace.depth = ++current->curr_ret_depth;
kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 196
Quoted 10 lines
kernel/trace/trace_functions_graph.c +3
-3/* Only trace if the calling function expects to */ if (!ftrace_graph_entry (&trace)) -return -EBUSY ; + goto out; -return ftrace_push_return_trace (ret, func, &trace.depth, - frame_pointer, retp); +if (ftrace_push_return_trace (ret, func, + frame_pointer, retp))
You can unwrap that line, by my counting that's at 69 chars, so unless you're editing on a C64 it should fit your screen.
kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 203
Quoted 7 lines
kernel/trace/trace_functions_graph.c +6
-0+ goto out; + +return 0 ; + out: + current->curr_ret_depth--; +return -EBUSY ; }
[diff "default"]
xfuncname = "^[[:alpha:]$_].*[^:]$"
avoids the need for that ludicrous label indenting.
Also, "error" might be a better label name.
Steven Rostedt
maintainerRe: [for-next][PATCH 15/18] function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
Quoted 13 lines · On Thu, 22 Nov 2018 11:03:22 +0100 Peter Zijlstra <[email protected]> wrote:
Quoted 12 lines · Steven Rostedt
kernel/trace/trace_functions_graph.c +2
-2@@ -188,14 +186,20 @@ int function_graph_enter(unsigned long ret, unsigned long func,struct ftrace_graph_ent trace; trace.func = func; - trace.depth = current->curr_ret_stack +1 ; + trace.depth = ++current->curr_ret_depth;/* Only trace if the calling function expects to */ if (!ftrace_graph_entry (&trace)) -return -EBUSY ; + goto out;
Quoted 7 lines
kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 196
Quoted 4 lines
kernel/trace/trace_functions_graph.c +2
-2-return ftrace_push_return_trace (ret, func, &trace.depth, - frame_pointer, retp); +if (ftrace_push_return_trace (ret, func, + frame_pointer, retp))You can unwrap that line, by my counting that's at 69 chars, so unless you're editing on a C64 it should fit your screen.
Thanks I may add a clean up patch. I already ran this code under several hours of testing, and only plan on touching it if there's more than cosmetic issues.
The reason that looks like that is because I just massaged the line it replaced, and didn't take into account that I could now make it into a single line.
Quoted 10 lines
kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 203
Quoted 8 lines
kernel/trace/trace_functions_graph.c +6
-0+ goto out; + +return 0 ; + out: + current->curr_ret_depth--; +return -EBUSY ; }[diff "default"]
Quoted 3 lines
xfuncname = "^[[:alpha:]$_].*[^:]$"avoids the need for that ludicrous label indenting.
I still prefer the 'space', and it fits with the rest of the file ;-)
TomAYto / TomAHto
Quoted 2 lines
Also, "error" might be a better label name.
True. I'll add that on top too, but for the stable/rc release, that isn't needed.
Thanks for taking a look at this code.
-- Steve
Steven Rostedt
maintainerPATCH 16/18 function_graph: Move return callback before update of curr_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
In the past, curr_ret_stack had two functions. One was to denote the depth of the call graph, the other is to keep track of where on the ret_stack the data is used. Although they may be slightly related, there are two cases where they need to be used differently.
The one case is that it keeps the ret_stack data from being corrupted by an interrupt coming in and overwriting the data still in use. The other is just to know where the depth of the stack currently is.
The function profiler uses the ret_stack to save a "subtime" variable that is part of the data on the ret_stack. If curr_ret_stack is modified too early, then this variable can be corrupted.
The "max_depth" option, when set to 1, will record the first functions going into the kernel. To see all top functions (when dealing with timings), the depth variable needs to be lowered before calling the return hook. But by lowering the curr_ret_stack, it makes the data on the ret_stack still being used by the return hook susceptible to being overwritten.
Now that there's two variables to handle both cases (curr_ret_depth), we can move them to the locations where they can handle both cases.
--- kernel/trace/trace_functions_graph.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
kernel/trace/trace_functions_graph.c +13 -9
@@ -261,7 +261,13 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret, trace->func = current->ret_stack[index].func; trace->calltime = current->ret_stack[index].calltime; trace->overrun =atomic_read (¤t->trace_overrun); - trace->depth = current->curr_ret_depth; + trace->depth = current->curr_ret_depth--; +/* +* We still want to trace interrupts coming in if +* max_depth is set to 1. Make sure the decrement is +* seen before ftrace_graph_return. +*/ +barrier (); }/*
@@ -275,9 +281,14 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)ftrace_pop_return_trace (&trace, &ret, frame_pointer); trace.rettime =trace_clock_local (); +ftrace_graph_return (&trace); +/* +* The ftrace_graph_return() may still access the current +* ret_stack structure, we need to make sure the update of +* curr_ret_stack is after that. +*/ barrier (); current->curr_ret_stack--; - current->curr_ret_depth--;/* * The curr_ret_stack can be less than -1 only if it was * filtered out and it's about to return from the function.
@@ -288,13 +299,6 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)return ret; } -/* -* The trace should run after decrementing the ret counter -* in case an interrupt were to come in. We don't want to -* lose the interrupt if max_depth is set. -*/ -ftrace_graph_return (&trace); -if (unlikely (!ret)) {ftrace_graph_stop ();WARN_ON (1 );
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 17/18 function_graph: Reverse the order of pushing the ret_stack and the callback
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function graph profiler uses the ret_stack to store the "subtime" and reuse it by nested functions and also on the return. But the current logic has the profiler callback called before the ret_stack is updated, and it is just modifying the ret_stack that will later be allocated (it's just lucky that the "subtime" is not touched when it is allocated).
This could also cause a crash if we are at the end of the ret_stack when this happens.
By reversing the order of the allocating the ret_stack and then calling the callbacks attached to a function being traced, the ret_stack entry is no longer used before it is allocated.
--- kernel/trace/trace_functions_graph.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/trace/trace_functions_graph.c +6 -4
@@ -188,15 +188,17 @@ int function_graph_enter(unsigned long ret, unsigned long func, trace.func = func; trace.depth = ++current->curr_ret_depth; -/* Only trace if the calling function expects to */ -if (!ftrace_graph_entry (&trace)) - goto out; -if (ftrace_push_return_trace (ret, func, frame_pointer, retp)) goto out; +/* Only trace if the calling function expects to */ +if (!ftrace_graph_entry (&trace)) + goto out_ret; +return 0 ; + out_ret: + current->curr_ret_stack--; out: current->curr_ret_depth--;return -EBUSY ;
Signature · 1 line
2.19.1
Steven Rostedt
maintainerPATCH 18/18 function_graph: Have profiler use curr_ret_stack and not depth
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The profiler uses trace->depth to find its entry on the ret_stack, but the depth may not match the actual location of where its entry is (if an interrupt were to preempt the processing of the profiler for another function, the depth and the curr_ret_stack will be different).
Have it use the curr_ret_stack as the index to find its ret_stack entry instead of using the depth variable, as that is no longer guaranteed to be the same.
--- kernel/trace/ftrace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/trace/ftrace.c +2 -2
@@ -817,7 +817,7 @@ function_profile_call(unsigned long ip, unsigned long parent_ip,#ifdef CONFIG_FUNCTION_GRAPH_TRACER static int profile_graph_entry (struct ftrace_graph_ent *trace) { -int index = trace->depth; +int index = current->curr_ret_stack;function_profile_call (trace->func,0 ,NULL ,NULL );
@@ -852,7 +852,7 @@ static void profile_graph_return(struct ftrace_graph_ret *trace)if (!fgraph_graph_time) {int index; - index = trace->depth; + index = current->curr_ret_stack;/* Append this call time to the parent time to subtract */ if (index)
Signature · 1 line
2.19.1
Hi Steve,
This series looks good to me.
for this series.
Thank you!
# Now I know why sometimes func-graph depth looks broken...
Quoted 90 lines · On Wed, 21 Nov 2018 19:28:01 -0500 Steven Rostedt <[email protected]> wrote:
While working on rewriting function graph tracer, I found a design flaw in the code. Commit
03274a3ffb449("tracing/fgraph: Adjust fgraph depth before calling trace return callback") tried to fix a bug that caused interrupts not to be traced ifmax_depthwas set to 1 (for debuggingNO_HZ_FULL), because if it came in just as another function that was being traced that had a depth of 1 was exiting. This caused the timing of being in the kernel to be off. The fix was simply to move the calling of the return function after the updating ofcurr_ret_stackindex as that was what was being used.The change log even says that it was safe to do this, but unfortunately it was not, and the
barrier()there was specifically *for* that function (which shows why one should document barriers).The problem is that the return callback may still be using what's on the shadow stack and by changing the shadow stack pointer, it may allow for another function being traced to overwrite that data. Note, if this happens, it will only cause garbage data to be traced and will not affect the actual operations of the kernel (ie. it wont crash).
Unfortunately just reverting that will bring back the old bug. The real way to fix that old bug is to create another counter to handle the depth, but this means that we need to change all the architectures that implement function graph tracing (that's 12 of them). Luckily, I need to do this anyway in my re-design so this is a good thing.
Since all the archictectures do basicall the same thing to prepare the function graph trace to be traced, I made a generic function that they all can use and simplified the logic of all the architectures. Then I'm able to fix the design problem in one place.
I pushed this code up to let zero-day have a whack at it, and I also downloaded the latest 8.1.0 cross compilers for all the archs that are affected and compiled tested them all (and got rid of all the warnings I introduced as well).
I marked this all for stable, but in reality it may not need to be ported as it will probably not be trivial to do so, becaues you'll need to also fix the architectures that are no longer used (although do we care about them?). But if someone really cares about correct timings of the function graph profiler when the options/graph-time is set to zero, then be my guest.
Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/urgentHead SHA1:
1ffd62e282a649483afb0bb4d76f7244b3c10075Steven Rostedt (VMware) (18):
function_graph: Createfunction_graph_enter()to consolidate architecture code x86/function_graph: Simplify withfunction_graph_entry()
ARM:function_graph: Simplify withfunction_graph_entry()
arm64:function_graph: Simplify withfunction_graph_entry()
microblaze:function_graph: Simplify withfunction_graph_entry()
MIPS:function_graph: Simplify withfunction_graph_entry()
nds32:function_graph: Simplify withfunction_graph_entry()
parisc:function_graph: Simplify withfunction_graph_entry()powerpc/function_graph: Simplify withfunction_graph_entry()riscv/function_graph: Simplify withfunction_graph_entry()s390/function_graph: Simplify withfunction_graph_entry()sh/function_graph: Simplify withfunction_graph_entry()sparc/function_graph: Simplify withfunction_graph_entry()
function_graph: Makeftrace_push_return_trace()static
function_graph: Use newcurr_ret_depthto manage depth instead ofcurr_ret_stack
function_graph: Move return callback before update ofcurr_ret_stack
function_graph: Reverse the order of pushing theret_stackand the callback
function_graph: Have profiler usecurr_ret_stackand not depth----
arch/arm/kernel/ftrace.c | 17 +------------ arch/arm64/kernel/ftrace.c | 15 +---------- arch/microblaze/kernel/ftrace.c | 15 ++--------- arch/mips/kernel/ftrace.c | 14 ++--------- arch/nds32/kernel/ftrace.c | 18 ++----------- arch/parisc/kernel/ftrace.c | 17 +++---------- arch/powerpc/kernel/trace/ftrace.c | 15 ++--------- arch/riscv/kernel/ftrace.c | 14 ++--------- arch/s390/kernel/ftrace.c | 13 ++-------- arch/sh/kernel/ftrace.c | 16 ++---------- arch/sparc/kernel/ftrace.c | 11 +------- arch/x86/kernel/ftrace.c | 15 +---------- include/linux/ftrace.h | 4 +-- include/linux/sched.h | 1 + kernel/trace/ftrace.c | 7 ++++-- kernel/trace/trace_functions_graph.c | 49 ++++++++++++++++++++++++++++-------- 16 files changed, 67 insertions(+), 174 deletions(-)
Signature · 1 line
Masami Hiramatsu <[email protected]>
Quoted 8 lines · Steven Rostedt
[ ... snip ... ]
Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/urgent
Hi Steve,
With your ftrace/urgent branch linked above, if I try a quick function_graph test like the following:
SYSFS=/sys/kernel/debug/tracing
echo 0 > "$SYSFS/tracing_on"
echo cmdline_proc_show > "$SYSFS/set_graph_function" echo function_graph > "$SYSFS/current_tracer" echo 1 > "$SYSFS/tracing_on"
I see a bunch of scheduler interrupt functions in the trace/trace_pipe without even invoking cmdline_proc_show().
This tests works as expected with Linux 4.20-rc3 though:
% cat /sys/kernel/debug/tracing/trace_pipe
2) | cmdline_proc_show() {
2) 0.320 us | seq_puts();
2) 0.030 us | seq_putc();
2) 1.352 us | }Operator error, or did the patchset break something?
Regards,
-- Joe
Steven Rostedt
maintainerQuoted 17 lines · On Wed, 28 Nov 2018 15:39:31 -0500 Joe Lawrence <[email protected]> wrote:
Hi Steve,
With your ftrace/urgent branch linked above, if I try a quick
function_graphtest like the following:SYSFS=/sys/kernel/debug/tracing
echo 0 > "$SYSFS/tracing_on"
echocmdline_proc_show> "$SYSFS/set_graph_function" echofunction_graph> "$SYSFS/current_tracer" echo 1 > "$SYSFS/tracing_on"I see a bunch of scheduler interrupt functions in the trace/trace_pipe without even invoking
cmdline_proc_show().This tests works as expected with Linux 4.20-rc3 though:
Quoted 7 lines
% cat
/sys/kernel/debug/tracing/trace_pipe2) | cmdline_proc_show() { 2) 0.320 us | seq_puts(); 2) 0.030 us | seq_putc(); 2) 1.352 us | }Operator error, or did the patchset break something?
Nope, that does seem to be a bug :-/
-- Steve
Steven Rostedt
maintainerQuoted 26 lines · On Wed, 28 Nov 2018 16:00:21 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 23 lines · On Wed, 28 Nov 2018 15:39:31 -0500 Joe Lawrence <[email protected]> wrote:
Hi Steve,
With your ftrace/urgent branch linked above, if I try a quick
function_graphtest like the following:SYSFS=/sys/kernel/debug/tracing
echo 0 > "$SYSFS/tracing_on"
echocmdline_proc_show> "$SYSFS/set_graph_function" echofunction_graph> "$SYSFS/current_tracer" echo 1 > "$SYSFS/tracing_on"I see a bunch of scheduler interrupt functions in the trace/trace_pipe without even invoking
cmdline_proc_show().This tests works as expected with Linux 4.20-rc3 though:
% cat
/sys/kernel/debug/tracing/trace_pipe2) | cmdline_proc_show() { 2) 0.320 us | seq_puts(); 2) 0.030 us | seq_putc(); 2) 1.352 us | }
Quoted 4 lines
Quoted 1 line
Operator error, or did the patchset break something?
Nope, that does seem to be a bug :-/
Does this patch fix it for you?
-- Steve
kernel/trace/trace.h +7 -1
@@ -512,6 +512,9 @@ enum {* can only be modified by current, we can reuse trace_recursion. */ TRACE_IRQ_BIT , + +/* Set if the function is in the set_graph_function file */ + TRACE_GRAPH_BIT, };#define trace_recursion_set (bit) do { (current)->trace_recursion |= (1<<(bit)); } while (0)
@@ -855,6 +858,8 @@ static inline int ftrace_graph_addr(unsigned long addr) }if (ftrace_lookup_ip (ftrace_graph_hash, addr)) { + +trace_recursion_set (TRACE_GRAPH_BIT );/* * If no irqs are to be traced, but a set_graph_function * is set, and called by an interrupt handler, we still
@@ -901,7 +906,8 @@ extern unsigned int fgraph_max_depth;static inline bool ftrace_graph_ignore_func (struct ftrace_graph_ent *trace) {/* trace it when it is-nested-in or is a function enabled. */ -return !(trace->depth ||ftrace_graph_addr (trace->func)) || +return !(trace_recursion_test (TRACE_GRAPH_BIT ) || +ftrace_graph_addr (trace->func)) || (trace->depth <0 ) || (fgraph_max_depth && trace->depth >= fgraph_max_depth); }
kernel/trace/trace_functions_graph.c +5 -0
@@ -509,6 +509,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace)int cpu;int pc; +if (!trace->depth) +trace_recursion_clear (TRACE_GRAPH_BIT ); +local_irq_save (flags); cpu =raw_smp_processor_id (); data =per_cpu_ptr (tr->trace_buffer.data, cpu);
@@ -532,6 +535,8 @@ void set_graph_array(struct trace_array *tr)static void trace_graph_thresh_return (struct ftrace_graph_ret *trace) { +if (!trace->depth) +trace_recursion_clear (TRACE_GRAPH_BIT );if (tracing_thresh && (trace->rettime - trace->calltime < tracing_thresh))return ;
kernel/trace/trace_irqsoff.c +3 -0
@@ -208,6 +208,9 @@ static void irqsoff_graph_return(struct ftrace_graph_ret *trace)unsigned long flags;int pc; +if (!trace->depth) +trace_recursion_clear (TRACE_GRAPH_BIT ); +if (!func_prolog_dec (tr, &data, &flags))return ;
kernel/trace/trace_sched_wakeup.c +3 -0
@@ -270,6 +270,9 @@ static void wakeup_graph_return(struct ftrace_graph_ret *trace)unsigned long flags;int pc; +if (!trace->depth) +trace_recursion_clear (TRACE_GRAPH_BIT ); +if (!func_prolog_preempt_disable (tr, &data, &pc))return ;
Steven Rostedt
maintainerQuoted 1 line · On Wed, 28 Nov 2018 22:29:36 -0500 Steven Rostedt <[email protected]> wrote:
Does this patch fix it for you?
Take 2. I realized that the reason for the interrupts being traced is because it is more likely to interrupt when the depth is already 0, setting it to 1 causing the interrupt to think it's already in a function that wants to be traced (when that wasn't decided yet).
The fix uses a bit that gets set when we want to trace (and will trace till that bit is cleared). That bit gets cleared on the return function when depth is zero. But if we are tracing something in a interrupt that happened to interrupt the beginning of a function that already set the depth to 0, then we need to clear the bit at depth of 1 not zero (and 2 if we want to trace a function that interrupted a softirq, that interrupted a start of normal function. and 3 if we want to trace an NMI function that had the very unlikely case of interrupting a start of a interrupt function, that interrupted the start of a softirq function, that interrupted a start of a normal context function!).
If that happens then we will continue to trace functions when we don't want to. To solve that, I need to record the depth (0-3) when the bit is set, and clear it when the return function is at that same depth (0-3).
-- Steve
kernel/trace/trace.h +54 -3
@@ -512,12 +512,44 @@ enum {* can only be modified by current, we can reuse trace_recursion. */ TRACE_IRQ_BIT , + +/* Set if the function is in the set_graph_function file */ +TRACE_GRAPH_BIT , + +/* +* In the very unlikely case that an interrupt came in +* at a start of graph tracing, and we want to trace +* the function in that interrupt, the depth can be greater +* than zero, because of the preempted start of a previous +* trace. In an even more unlikely case, depth could be 2 +* if a softirq interrupted the start of graph tracing, +* followed by an interrupt preempting a start of graph +* tracing in the softirq, and depth can even be 3 +* if an NMI came in at the start of an interrupt function +* that preempted a softirq start of a function that +* preempted normal context!!!! Luckily, it can't be +* greater than 3, so the next two bits are a mask +* of what the depth is when we set TRACE_GRAPH_BIT +*/ + +TRACE_GRAPH_DEPTH_START_BIT , + TRACE_GRAPH_DEPTH_END_BIT, };#define trace_recursion_set (bit) do { (current)->trace_recursion |= (1<<(bit)); } while (0)#define trace_recursion_clear (bit) do { (current)->trace_recursion &= ~(1<<(bit)); } while (0)#define trace_recursion_test (bit) ((current)->trace_recursion & (1<<(bit))) +#define trace_recursion_depth () \ + (((current)->trace_recursion >> TRACE_GRAPH_DEPTH_START_BIT) & 3) +#define trace_recursion_set_depth (depth) \ + do { \ + current->trace_recursion &= \ + ~(3 << TRACE_GRAPH_DEPTH_START_BIT); \ + current->trace_recursion |= \ + ((depth) & 3) << TRACE_GRAPH_DEPTH_START_BIT; \ + } while (0) +#define TRACE_CONTEXT_BITS 4#define TRACE_FTRACE_START TRACE_FTRACE_BIT
@@ -843,8 +875,9 @@ extern void __trace_graph_return(struct trace_array *tr,extern struct ftrace_hash *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash; -static inline int ftrace_graph_addr (unsigned long addr) +static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) { +unsigned long addr = trace->func;int ret =0 ;preempt_disable_notrace ();
@@ -855,6 +888,14 @@ static inline int ftrace_graph_addr(unsigned long addr) }if (ftrace_lookup_ip (ftrace_graph_hash, addr)) { + +/* +* This needs to be cleared on the return functions +* when the depth is zero. +*/ +trace_recursion_set (TRACE_GRAPH_BIT ); +trace_recursion_set_depth (trace->depth); +/* * If no irqs are to be traced, but a set_graph_function * is set, and called by an interrupt handler, we still
@@ -872,6 +913,13 @@ static inline int ftrace_graph_addr(unsigned long addr)return ret; } +static inline void ftrace_graph_addr_finish (struct ftrace_graph_ret *trace) +{ +if (trace_recursion_test (TRACE_GRAPH_BIT ) && + trace->depth ==trace_recursion_depth ()) +trace_recursion_clear (TRACE_GRAPH_BIT ); +} +static inline int ftrace_graph_notrace_addr (unsigned long addr) {int ret =0 ;
@@ -885,7 +933,7 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr) return ret; }#else -static inline int ftrace_graph_addr (unsigned long addr) +static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {return 1 ; }
@@ -894,6 +942,8 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr) {return 0 ; } +static inline void ftrace_graph_addr_finish (struct ftrace_graph_ret *trace) +{ }#endif /* CONFIG_DYNAMIC_FTRACE */ extern unsigned int fgraph_max_depth;
@@ -901,7 +951,8 @@ extern unsigned int fgraph_max_depth;static inline bool ftrace_graph_ignore_func (struct ftrace_graph_ent *trace) {/* trace it when it is-nested-in or is a function enabled. */ -return !(trace->depth ||ftrace_graph_addr (trace->func)) || +return !(trace_recursion_test (TRACE_GRAPH_BIT ) || +ftrace_graph_addr (trace)) || (trace->depth <0 ) || (fgraph_max_depth && trace->depth >= fgraph_max_depth); }
kernel/trace/trace_functions_graph.c +4 -0
@@ -509,6 +509,8 @@ void trace_graph_return(struct ftrace_graph_ret *trace)int cpu;int pc; +ftrace_graph_addr_finish (trace); +local_irq_save (flags); cpu =raw_smp_processor_id (); data =per_cpu_ptr (tr->trace_buffer.data, cpu);
@@ -532,6 +534,8 @@ void set_graph_array(struct trace_array *tr)static void trace_graph_thresh_return (struct ftrace_graph_ret *trace) { +ftrace_graph_addr_finish (trace); +if (tracing_thresh && (trace->rettime - trace->calltime < tracing_thresh))return ;
kernel/trace/trace_irqsoff.c +2 -0
@@ -208,6 +208,8 @@ static void irqsoff_graph_return(struct ftrace_graph_ret *trace)unsigned long flags;int pc; +ftrace_graph_addr_finish (trace); +if (!func_prolog_dec (tr, &data, &flags))return ;
kernel/trace/trace_sched_wakeup.c +2 -0
@@ -270,6 +270,8 @@ static void wakeup_graph_return(struct ftrace_graph_ret *trace)unsigned long flags;int pc; +ftrace_graph_addr_finish (trace); +if (!func_prolog_preempt_disable (tr, &data, &pc))return ;
Quoted 187 lines · Steven Rostedt
Quoted 1 line · On Wed, 28 Nov 2018 22:29:36 -0500 Steven Rostedt <[email protected]> wrote:
Does this patch fix it for you?
Take 2. I realized that the reason for the interrupts being traced is because it is more likely to interrupt when the depth is already 0, setting it to 1 causing the interrupt to think it's already in a function that wants to be traced (when that wasn't decided yet).
The fix uses a bit that gets set when we want to trace (and will trace till that bit is cleared). That bit gets cleared on the return function when depth is zero. But if we are tracing something in a interrupt that happened to interrupt the beginning of a function that already set the depth to 0, then we need to clear the bit at depth of 1 not zero (and 2 if we want to trace a function that interrupted a softirq, that interrupted a start of normal function. and 3 if we want to trace an NMI function that had the very unlikely case of interrupting a start of a interrupt function, that interrupted the start of a softirq function, that interrupted a start of a normal context function!).
If that happens then we will continue to trace functions when we don't want to. To solve that, I need to record the depth (0-3) when the bit is set, and clear it when the return function is at that same depth (0-3).
-- Steve
kernel/trace/trace.h +54
-3@@ -512,12 +512,44 @@ enum {* can only be modified by current, we can reuse trace_recursion. */ TRACE_IRQ_BIT , + +/* Set if the function is in the set_graph_function file */ +TRACE_GRAPH_BIT , + +/* +* In the very unlikely case that an interrupt came in +* at a start of graph tracing, and we want to trace +* the function in that interrupt, the depth can be greater +* than zero, because of the preempted start of a previous +* trace. In an even more unlikely case, depth could be 2 +* if a softirq interrupted the start of graph tracing, +* followed by an interrupt preempting a start of graph +* tracing in the softirq, and depth can even be 3 +* if an NMI came in at the start of an interrupt function +* that preempted a softirq start of a function that +* preempted normal context!!!! Luckily, it can't be +* greater than 3, so the next two bits are a mask +* of what the depth is when we set TRACE_GRAPH_BIT +*/ + +TRACE_GRAPH_DEPTH_START_BIT , + TRACE_GRAPH_DEPTH_END_BIT, };#define trace_recursion_set (bit) do { (current)->trace_recursion |= (1<<(bit)); } while (0)#define trace_recursion_clear (bit) do { (current)->trace_recursion &= ~(1<<(bit)); } while (0)#define trace_recursion_test (bit) ((current)->trace_recursion & (1<<(bit))) +#define trace_recursion_depth () \ + (((current)->trace_recursion >> TRACE_GRAPH_DEPTH_START_BIT) & 3) +#define trace_recursion_set_depth (depth) \ + do { \ + current->trace_recursion &= \ + ~(3 << TRACE_GRAPH_DEPTH_START_BIT); \ + current->trace_recursion |= \ + ((depth) & 3) << TRACE_GRAPH_DEPTH_START_BIT; \ + } while (0) +#define TRACE_CONTEXT_BITS 4#define TRACE_FTRACE_START TRACE_FTRACE_BIT@@ -843,8 +875,9 @@ extern void __trace_graph_return(struct trace_array *tr,extern struct ftrace_hash *ftrace_graph_hash;extern struct ftrace_hash *ftrace_graph_notrace_hash; -static inline int ftrace_graph_addr (unsigned long addr) +static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) { +unsigned long addr = trace->func;int ret =0 ;preempt_disable_notrace ();@@ -855,6 +888,14 @@ static inline int ftrace_graph_addr(unsigned long addr) }if (ftrace_lookup_ip (ftrace_graph_hash, addr)) { + +/* +* This needs to be cleared on the return functions +* when the depth is zero. +*/ +trace_recursion_set (TRACE_GRAPH_BIT ); +trace_recursion_set_depth (trace->depth); +/* * If no irqs are to be traced, but a set_graph_function * is set, and called by an interrupt handler, we still @@ -872,6 +913,13 @@ static inline int ftrace_graph_addr(unsigned long addr)return ret; } +static inline void ftrace_graph_addr_finish (struct ftrace_graph_ret *trace) +{ +if (trace_recursion_test (TRACE_GRAPH_BIT ) && + trace->depth ==trace_recursion_depth ()) +trace_recursion_clear (TRACE_GRAPH_BIT ); +} +static inline int ftrace_graph_notrace_addr (unsigned long addr) {int ret =0 ;@@ -885,7 +933,7 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr) return ret; }#else -static inline int ftrace_graph_addr (unsigned long addr) +static inline int ftrace_graph_addr (struct ftrace_graph_ent *trace) {return 1 ; }@@ -894,6 +942,8 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr) {return 0 ; } +static inline void ftrace_graph_addr_finish (struct ftrace_graph_ret *trace) +{ }#endif /* CONFIG_DYNAMIC_FTRACE */ extern unsigned int fgraph_max_depth;@@ -901,7 +951,8 @@ extern unsigned int fgraph_max_depth;static inline bool ftrace_graph_ignore_func (struct ftrace_graph_ent *trace) {/* trace it when it is-nested-in or is a function enabled. */ -return !(trace->depth ||ftrace_graph_addr (trace->func)) || +return !(trace_recursion_test (TRACE_GRAPH_BIT ) || +ftrace_graph_addr (trace)) || (trace->depth <0 ) || (fgraph_max_depth && trace->depth >= fgraph_max_depth); }kernel/trace/trace_functions_graph.c +4
-0@@ -509,6 +509,8 @@ void trace_graph_return(struct ftrace_graph_ret *trace)int cpu;int pc; +ftrace_graph_addr_finish (trace); +local_irq_save (flags); cpu =raw_smp_processor_id (); data =per_cpu_ptr (tr->trace_buffer.data, cpu);@@ -532,6 +534,8 @@ void set_graph_array(struct trace_array *tr)static void trace_graph_thresh_return (struct ftrace_graph_ret *trace) { +ftrace_graph_addr_finish (trace); +if (tracing_thresh && (trace->rettime - trace->calltime < tracing_thresh))return ;kernel/trace/trace_irqsoff.c +2
-0@@ -208,6 +208,8 @@ static void irqsoff_graph_return(struct ftrace_graph_ret *trace)unsigned long flags;int pc; +ftrace_graph_addr_finish (trace); +if (!func_prolog_dec (tr, &data, &flags))return ;kernel/trace/trace_sched_wakeup.c +0
-0
kernel/trace/trace_sched_wakeup.c static void wakeup_graph_return(struct ftrace_graph_ret *trace) · line 277
Quoted 8 lines
kernel/trace/trace_sched_wakeup.c +2
-0@@ -270,6 +270,8 @@ static void wakeup_graph_return(struct ftrace_graph_ret *trace)unsigned long flags;int pc; +ftrace_graph_addr_finish (trace); +if (!func_prolog_preempt_disable (tr, &data, &pc))return ;
With the "take 2" patch, I only see smp_irq_work_interrupt() graph when the graph_function is in progress.. for example:
3) | cmdline_proc_show() {
3) ==========> |
3) | smp_irq_work_interrupt() {
3) | irq_enter() {
3) | rcu_irq_enter() {
3) 0.141 us | rcu_dynticks_curr_cpu_in_eqs();
3) 0.589 us | }
3) 0.877 us | }
3) | __wake_up() {
3) | __wake_up_common_lock() {
3) 0.122 us | _raw_spin_lock_irqsave();
3) | __wake_up_common() {
3) | autoremove_wake_function() {
3) | default_wake_function() {
3) | try_to_wake_up() {
3) 0.209 us | _raw_spin_lock_irqsave();
3) | select_task_rq_fair() {
3) | select_idle_sibling() {
3) 0.140 us | available_idle_cpu();
3) 0.408 us | }
3) 0.711 us | }
3) | native_smp_send_reschedule() {
3) 4.298 us | x2apic_send_IPI();
3) 4.574 us | }
3) 0.144 us | _raw_spin_unlock_irqrestore();
3) 6.677 us | }
3) 6.915 us | }
3) 7.182 us | }
3) 7.620 us | }
3) 0.132 us | _raw_spin_unlock_irqrestore();
3) 8.401 us | }
3) 9.066 us | }
3) | irq_exit() {
3) 0.128 us | idle_cpu();
3) | rcu_irq_exit() {
3) 0.122 us | rcu_dynticks_curr_cpu_in_eqs();
3) + 10.598 us | }
3) + 11.128 us | }
3) + 22.075 us | }
3) <========== |
3) 0.414 us | seq_puts();
3) 0.128 us | seq_putc();
3) + 29.040 us | }-- Joe
Steven Rostedt
maintainerQuoted 2 lines · On Thu, 29 Nov 2018 10:08:55 -0500 Joe Lawrence <[email protected]> wrote:
With the "take 2" patch, I only see
smp_irq_work_interrupt()graph when thegraph_functionis in progress.. for example:
In other words it works :-)
Can I get a Tested-by from you?
-- Steve
Joe Lawrence
Tested-byQuoted 10 lines · Steven Rostedt
Quoted 2 lines · On Thu, 29 Nov 2018 10:08:55 -0500 Joe Lawrence <[email protected]> wrote:
With the "take 2" patch, I only see
smp_irq_work_interrupt()graph when thegraph_functionis in progress.. for example:In other words it works :-)
Can I get a Tested-by from you?
Yup, thanks for fixing!
-- Joe