[PATCH] mips: function tracer: Fix broken function tracing

[PATCH] mips: function tracer: Fix broken function tracing

[Al Cooper]

Function tracing is currently broken for all 32 bit MIPS platforms.
When tracing is enabled, the kernel immediately hangs on boot.
This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0
that changes the kernel/trace/Kconfig file so that is no longer
forces FRAME_POINTER when FUNCTION_TRACING is enabled.

MIPS frame pointers are generally considered to be useless because
they cannot be used to unwind the stack. Unfortunately the MIPS
function tracing code has bugs that are masked by the use of frame
pointers. This commit fixes the bugs so that MIPS frame pointers do
not need to be enabled.

The bugs are a result of the odd calling sequence used to call the trace
routine. This calling sequence is inserted into every tracable function
when the tracing CONFIG option is enabled. This sequence is generated
for 32bit MIPS platforms by the compiler via the "-pg" flag.
Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
call to the trace routine "_mcount" (some legacy thing where 2 arguments
used to be pushed on the stack). The _mcount routine is expected to
adjust the sp by +8 before returning.

One of the bugs is that when tracing is disabled for a function, the
"jalr _mcount" instruction is replaced with a nop, but the
"addiu sp,sp,-8" is still executed and the stack pointer is left
trashed. When frame pointers are enabled the problem is masked
because any access to the stack is done through the frame
pointer and the stack pointer is restored from the frame pointer when
the function returns. This patch uses a branch likely instruction
"bltzl zero, f1" instead of "nop" to disable the call because this
instruction will not execute the "addiu sp,sp,-8" instruction in
the delay slot. The other possible solution would be to nop out both
the jalr and the "addiu sp,sp,-8", but this would need to be interrupt
and SMP safe and would be much more intrusive.

A few other bugs were fixed where the _mcount routine itself did not
always fix the sp on return.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 arch/mips/kernel/ftrace.c |    9 ++++++++-
 arch/mips/kernel/mcount.S |   14 ++++++++++----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 6a2d758..f761130 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -46,6 +46,13 @@ static inline int in_kernel_space(unsigned long ip)
 #define JUMP_RANGE_MASK ((1UL << 28) - 1)
 
 #define INSN_NOP 0x00000000	/* nop */
+
+/*
+ * This branch likely instruction is used to nop the call to _mcount
+ * and skip the stack adjust instruction in the delay slot.
+ */
+#define INSN_NOP_SKIP 0x04020001	/* bltzl zero, f1 */
+
 #define INSN_JAL(addr)	\
 	((unsigned int)(JAL | (((addr) >> 2) & ADDR_MASK)))
 
@@ -130,7 +137,7 @@ int ftrace_make_nop(struct module *mod,
 	 * If ip is in kernel space, no long call, otherwise, long call is
 	 * needed.
 	 */
-	new = in_kernel_space(ip) ? INSN_NOP : INSN_B_1F;
+	new = in_kernel_space(ip) ? INSN_NOP_SKIP : INSN_B_1F;
 
 	return ftrace_modify_code(ip, new);
 }
diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
index 4c968e7..78ac3cc 100644
--- a/arch/mips/kernel/mcount.S
+++ b/arch/mips/kernel/mcount.S
@@ -68,10 +68,10 @@
 NESTED(ftrace_caller, PT_SIZE, ra)
 	.globl _mcount
 _mcount:
-	b	ftrace_stub
+	b	ftrace_stub_restore_sp
 	 nop
 	lw	t1, function_trace_stop
-	bnez	t1, ftrace_stub
+	bnez	t1, ftrace_stub_restore_sp
 	 nop
 
 	MCOUNT_SAVE_REGS
@@ -96,13 +96,16 @@ ftrace_graph_call:
 	.globl ftrace_stub
 ftrace_stub:
 	RETURN_BACK
+ftrace_stub_restore_sp:
+	PTR_ADDIU	sp, 8
+	RETURN_BACK
 	END(ftrace_caller)
 
 #else	/* ! CONFIG_DYNAMIC_FTRACE */
 
 NESTED(_mcount, PT_SIZE, ra)
 	lw	t1, function_trace_stop
-	bnez	t1, ftrace_stub
+	bnez	t1, ftrace_stub_restore_sp
 	 nop
 	PTR_LA	t1, ftrace_stub
 	PTR_L	t2, ftrace_trace_function /* Prepare t2 for (1) */
@@ -118,7 +121,7 @@ NESTED(_mcount, PT_SIZE, ra)
 	bne	t1, t3, ftrace_graph_caller
 	 nop
 #endif
-	b	ftrace_stub
+	b	ftrace_stub_restore_sp
 	 nop
 
 static_trace:
@@ -132,6 +135,9 @@ static_trace:
 	.globl ftrace_stub
 ftrace_stub:
 	RETURN_BACK
+ftrace_stub_restore_sp:
+	PTR_ADDIU	sp, 8
+	RETURN_BACK
 	END(_mcount)
 
 #endif	/* ! CONFIG_DYNAMIC_FTRACE */
-- 
1.7.6

Re: [PATCH] mips: function tracer: Fix broken function tracing

[David Daney]

On 01/11/2013 06:33 AM, Al Cooper wrote:
> Function tracing is currently broken for all 32 bit MIPS platforms.
> When tracing is enabled, the kernel immediately hangs on boot.
> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0
> that changes the kernel/trace/Kconfig file so that is no longer
> forces FRAME_POINTER when FUNCTION_TRACING is enabled.
>
> MIPS frame pointers are generally considered to be useless because
> they cannot be used to unwind the stack. Unfortunately the MIPS
> function tracing code has bugs that are masked by the use of frame
> pointers. This commit fixes the bugs so that MIPS frame pointers do
> not need to be enabled.
>
> The bugs are a result of the odd calling sequence used to call the trace
> routine. This calling sequence is inserted into every tracable function
> when the tracing CONFIG option is enabled. This sequence is generated
> for 32bit MIPS platforms by the compiler via the "-pg" flag.
> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
> call to the trace routine "_mcount" (some legacy thing where 2 arguments
> used to be pushed on the stack). The _mcount routine is expected to
> adjust the sp by +8 before returning.
>
> One of the bugs is that when tracing is disabled for a function, the
> "jalr _mcount" instruction is replaced with a nop, but the
> "addiu sp,sp,-8" is still executed and the stack pointer is left
> trashed. When frame pointers are enabled the problem is masked
> because any access to the stack is done through the frame
> pointer and the stack pointer is restored from the frame pointer when
> the function returns. This patch uses a branch likely instruction
> "bltzl zero, f1" instead of "nop" to disable the call because this
> instruction will not execute the "addiu sp,sp,-8" instruction in
> the delay slot. The other possible solution would be to nop out both
> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt
> and SMP safe and would be much more intrusive.

I thought all CPUs were in stop_machine() when the modifications were 
done, so that there is no issue with multi-word instruction patching.

Am I wrong about this?

So really I think you can do two NOP just as easily.

The only reason I bring this up is that I am not sure all 32-bit CPUs 
implement the 'Likely' branch variants. Also there may be an affect on 
the branch predictor.

A third possibility would be to replace the JALR with 'ADDIU SP,SP,8' 
That way the following adjustment would be canceled out.  This would 
work well on single-issue CPUs, but the instructions may not be able to 
dual-issue on a multi issue machine due to data dependencies.

David Daney

>
> A few other bugs were fixed where the _mcount routine itself did not
> always fix the sp on return.
>

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Alan Cooper]

I already tried using "adddiu sp, sp, 8" and it caused the kernel to
randomly crash. After many hours of debugging the reason occurred to
me while in bed in the middle of the night. The problem is that if we
get an interrupt between the add 8 and the add -8 instructions, we
trash the existing stack.

The problem with the 2 nop approach is that there are a series of
subroutines used to write each nop and these nested subroutines are
traceable. This means on the second call to these subroutines they
execute with only one nop and crash. I could  write  some new code
that wrote the 2 nops at once, but (now that I understand
"stop_machine") with the branch likely solution we should be able to
stop using "stop_machine" when we write nops to the 20-30 thousand
Linux functions. It looks like other platforms have stopped using
stop_machine.

Al

On Fri, Jan 11, 2013 at 12:01 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 01/11/2013 06:33 AM, Al Cooper wrote:
>>
>> Function tracing is currently broken for all 32 bit MIPS platforms.
>> When tracing is enabled, the kernel immediately hangs on boot.
>> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0
>> that changes the kernel/trace/Kconfig file so that is no longer
>> forces FRAME_POINTER when FUNCTION_TRACING is enabled.
>>
>> MIPS frame pointers are generally considered to be useless because
>> they cannot be used to unwind the stack. Unfortunately the MIPS
>> function tracing code has bugs that are masked by the use of frame
>> pointers. This commit fixes the bugs so that MIPS frame pointers do
>> not need to be enabled.
>>
>> The bugs are a result of the odd calling sequence used to call the trace
>> routine. This calling sequence is inserted into every tracable function
>> when the tracing CONFIG option is enabled. This sequence is generated
>> for 32bit MIPS platforms by the compiler via the "-pg" flag.
>> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
>> call to the trace routine "_mcount" (some legacy thing where 2 arguments
>> used to be pushed on the stack). The _mcount routine is expected to
>> adjust the sp by +8 before returning.
>>
>> One of the bugs is that when tracing is disabled for a function, the
>> "jalr _mcount" instruction is replaced with a nop, but the
>> "addiu sp,sp,-8" is still executed and the stack pointer is left
>> trashed. When frame pointers are enabled the problem is masked
>> because any access to the stack is done through the frame
>> pointer and the stack pointer is restored from the frame pointer when
>> the function returns. This patch uses a branch likely instruction
>> "bltzl zero, f1" instead of "nop" to disable the call because this
>> instruction will not execute the "addiu sp,sp,-8" instruction in
>> the delay slot. The other possible solution would be to nop out both
>> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt
>> and SMP safe and would be much more intrusive.
>
>
> I thought all CPUs were in stop_machine() when the modifications were done,
> so that there is no issue with multi-word instruction patching.
>
> Am I wrong about this?
>
> So really I think you can do two NOP just as easily.
>
> The only reason I bring this up is that I am not sure all 32-bit CPUs
> implement the 'Likely' branch variants. Also there may be an affect on the
> branch predictor.
>
> A third possibility would be to replace the JALR with 'ADDIU SP,SP,8' That
> way the following adjustment would be canceled out.  This would work well on
> single-issue CPUs, but the instructions may not be able to dual-issue on a
> multi issue machine due to data dependencies.
>
> David Daney
>
>
>>
>> A few other bugs were fixed where the _mcount routine itself did not
>> always fix the sp on return.
>>
>

Re: [PATCH] mips: function tracer: Fix broken function tracing

[David Daney]

On 01/14/2013 01:10 PM, Alan Cooper wrote:
> I already tried using "adddiu sp, sp, 8" and it caused the kernel to
> randomly crash. After many hours of debugging the reason occurred to
> me while in bed in the middle of the night. The problem is that if we
> get an interrupt between the add 8 and the add -8 instructions, we
> trash the existing stack.
>
> The problem with the 2 nop approach is that there are a series of
> subroutines used to write each nop and these nested subroutines are
> traceable.

This seems like a bug.  The low-level code used to do code patching 
probably should be CFLAGS_REMOVE_file.o = -pg


> This means on the second call to these subroutines they
> execute with only one nop and crash. I could  write  some new code
> that wrote the 2 nops at once, but (now that I understand
> "stop_machine") with the branch likely solution we should be able to
> stop using "stop_machine" when we write nops to the 20-30 thousand
> Linux functions. It looks like other platforms have stopped using
> stop_machine.

I don't particularly object to the 'branch likely solution', but I think 
the failures of the other approaches indicates underlying bugs in the 
tracing code.  Those bugs should probably be fixed.

David Daney


>
> Al
>
> On Fri, Jan 11, 2013 at 12:01 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>> On 01/11/2013 06:33 AM, Al Cooper wrote:
>>>
>>> Function tracing is currently broken for all 32 bit MIPS platforms.
>>> When tracing is enabled, the kernel immediately hangs on boot.
>>> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0
>>> that changes the kernel/trace/Kconfig file so that is no longer
>>> forces FRAME_POINTER when FUNCTION_TRACING is enabled.
>>>
>>> MIPS frame pointers are generally considered to be useless because
>>> they cannot be used to unwind the stack. Unfortunately the MIPS
>>> function tracing code has bugs that are masked by the use of frame
>>> pointers. This commit fixes the bugs so that MIPS frame pointers do
>>> not need to be enabled.
>>>
>>> The bugs are a result of the odd calling sequence used to call the trace
>>> routine. This calling sequence is inserted into every tracable function
>>> when the tracing CONFIG option is enabled. This sequence is generated
>>> for 32bit MIPS platforms by the compiler via the "-pg" flag.
>>> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
>>> call to the trace routine "_mcount" (some legacy thing where 2 arguments
>>> used to be pushed on the stack). The _mcount routine is expected to
>>> adjust the sp by +8 before returning.
>>>
>>> One of the bugs is that when tracing is disabled for a function, the
>>> "jalr _mcount" instruction is replaced with a nop, but the
>>> "addiu sp,sp,-8" is still executed and the stack pointer is left
>>> trashed. When frame pointers are enabled the problem is masked
>>> because any access to the stack is done through the frame
>>> pointer and the stack pointer is restored from the frame pointer when
>>> the function returns. This patch uses a branch likely instruction
>>> "bltzl zero, f1" instead of "nop" to disable the call because this
>>> instruction will not execute the "addiu sp,sp,-8" instruction in
>>> the delay slot. The other possible solution would be to nop out both
>>> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt
>>> and SMP safe and would be much more intrusive.
>>
>>
>> I thought all CPUs were in stop_machine() when the modifications were done,
>> so that there is no issue with multi-word instruction patching.
>>
>> Am I wrong about this?
>>
>> So really I think you can do two NOP just as easily.
>>
>> The only reason I bring this up is that I am not sure all 32-bit CPUs
>> implement the 'Likely' branch variants. Also there may be an affect on the
>> branch predictor.
>>
>> A third possibility would be to replace the JALR with 'ADDIU SP,SP,8' That
>> way the following adjustment would be canceled out.  This would work well on
>> single-issue CPUs, but the instructions may not be able to dual-issue on a
>> multi issue machine due to data dependencies.
>>
>> David Daney
>>
>>
>>>
>>> A few other bugs were fixed where the _mcount routine itself did not
>>> always fix the sp on return.
>>>
>>
>
>

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Alan Cooper]

On Mon, Jan 14, 2013 at 5:12 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 01/14/2013 01:10 PM, Alan Cooper wrote:
>>
>> I already tried using "adddiu sp, sp, 8" and it caused the kernel to
>> randomly crash. After many hours of debugging the reason occurred to
>> me while in bed in the middle of the night. The problem is that if we
>> get an interrupt between the add 8 and the add -8 instructions, we
>> trash the existing stack.
>>
>> The problem with the 2 nop approach is that there are a series of
>> subroutines used to write each nop and these nested subroutines are
>> traceable.
>
>
> This seems like a bug.  The low-level code used to do code patching probably
> should be CFLAGS_REMOVE_file.o = -pg

While tracing mcount cannot be done because it's recursive, allowing
tracing of the code to enable/disable the call to mcount can be done
and seems useful. Also, fixing the 2 nop solution this way will still
not allow us to stop using stop_machine() which is hugely disruptive
to a running system. Remember that when tracing is enabled and
disabled we end up modifying 20 to 30 thousand functions. Moving this
functionality out of stop_machine() seems like a big benefit.

>
>
>
>> This means on the second call to these subroutines they
>> execute with only one nop and crash. I could  write  some new code
>> that wrote the 2 nops at once, but (now that I understand
>> "stop_machine") with the branch likely solution we should be able to
>> stop using "stop_machine" when we write nops to the 20-30 thousand
>> Linux functions. It looks like other platforms have stopped using
>> stop_machine.
>
>
> I don't particularly object to the 'branch likely solution', but I think the
> failures of the other approaches indicates underlying bugs in the tracing
> code.  Those bugs should probably be fixed.

If a solution can be found that modifies a single 32bit instruction to
enable/disable tracing, I don't see any bugs in the underlying code.
Plus we can avoid using stop_machine().

>
> David Daney
>
>
>
>>
>> Al
>>
>> On Fri, Jan 11, 2013 at 12:01 PM, David Daney <ddaney.cavm@gmail.com>
>> wrote:
>>>
>>> On 01/11/2013 06:33 AM, Al Cooper wrote:
>>>>
>>>>
>>>> Function tracing is currently broken for all 32 bit MIPS platforms.
>>>> When tracing is enabled, the kernel immediately hangs on boot.
>>>> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0
>>>> that changes the kernel/trace/Kconfig file so that is no longer
>>>> forces FRAME_POINTER when FUNCTION_TRACING is enabled.
>>>>
>>>> MIPS frame pointers are generally considered to be useless because
>>>> they cannot be used to unwind the stack. Unfortunately the MIPS
>>>> function tracing code has bugs that are masked by the use of frame
>>>> pointers. This commit fixes the bugs so that MIPS frame pointers do
>>>> not need to be enabled.
>>>>
>>>> The bugs are a result of the odd calling sequence used to call the trace
>>>> routine. This calling sequence is inserted into every tracable function
>>>> when the tracing CONFIG option is enabled. This sequence is generated
>>>> for 32bit MIPS platforms by the compiler via the "-pg" flag.
>>>> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
>>>> call to the trace routine "_mcount" (some legacy thing where 2 arguments
>>>> used to be pushed on the stack). The _mcount routine is expected to
>>>> adjust the sp by +8 before returning.
>>>>
>>>> One of the bugs is that when tracing is disabled for a function, the
>>>> "jalr _mcount" instruction is replaced with a nop, but the
>>>> "addiu sp,sp,-8" is still executed and the stack pointer is left
>>>> trashed. When frame pointers are enabled the problem is masked
>>>> because any access to the stack is done through the frame
>>>> pointer and the stack pointer is restored from the frame pointer when
>>>> the function returns. This patch uses a branch likely instruction
>>>> "bltzl zero, f1" instead of "nop" to disable the call because this
>>>> instruction will not execute the "addiu sp,sp,-8" instruction in
>>>> the delay slot. The other possible solution would be to nop out both
>>>> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt
>>>> and SMP safe and would be much more intrusive.
>>>
>>>
>>>
>>> I thought all CPUs were in stop_machine() when the modifications were
>>> done,
>>> so that there is no issue with multi-word instruction patching.
>>>
>>> Am I wrong about this?
>>>
>>> So really I think you can do two NOP just as easily.
>>>
>>> The only reason I bring this up is that I am not sure all 32-bit CPUs
>>> implement the 'Likely' branch variants. Also there may be an affect on
>>> the
>>> branch predictor.
>>>
>>> A third possibility would be to replace the JALR with 'ADDIU SP,SP,8'
>>> That
>>> way the following adjustment would be canceled out.  This would work well
>>> on
>>> single-issue CPUs, but the instructions may not be able to dual-issue on
>>> a
>>> multi issue machine due to data dependencies.
>>>
>>> David Daney
>>>
>>>
>>>>
>>>> A few other bugs were fixed where the _mcount routine itself did not
>>>> always fix the sp on return.
>>>>
>>>
>>
>>
>

Re: [PATCH] mips: function tracer: Fix broken function tracing

[David Daney]

On 01/14/2013 04:13 PM, Alan Cooper wrote:
> On Mon, Jan 14, 2013 at 5:12 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>> On 01/14/2013 01:10 PM, Alan Cooper wrote:
>>>
>>> I already tried using "adddiu sp, sp, 8" and it caused the kernel to
>>> randomly crash. After many hours of debugging the reason occurred to
>>> me while in bed in the middle of the night. The problem is that if we
>>> get an interrupt between the add 8 and the add -8 instructions, we
>>> trash the existing stack.
>>>
>>> The problem with the 2 nop approach is that there are a series of
>>> subroutines used to write each nop and these nested subroutines are
>>> traceable.
>>
>>
>> This seems like a bug.  The low-level code used to do code patching probably
>> should be CFLAGS_REMOVE_file.o = -pg
>
> While tracing mcount cannot be done because it's recursive, allowing
> tracing of the code to enable/disable the call to mcount can be done
> and seems useful. Also, fixing the 2 nop solution this way will still
> not allow us to stop using stop_machine() which is hugely disruptive
> to a running system. Remember that when tracing is enabled and
> disabled we end up modifying 20 to 30 thousand functions. Moving this
> functionality out of stop_machine() seems like a big benefit.


The main point of code patching is to have the Tracing Off state be very 
low overhead.  So low that it might be acceptable to leave it on in a 
production kernel.

Now imagine a highly pipe-lined CPU architecture with a good branch 
predictor.

Issuing a NOP instruction is very low overhead.  At most one cycle (a 
fraction of a cycle on a multi-issue CPU).  A Branch Likely that 
mispredicts (as all of these probably will) can incur a full pipeline 
flush which can be 10 or more cycles on some machines.  This is quite a 
hit for something that is supposed to be low overhead.

It is better than nothing I guess, but I think a better approach would 
be to modify GCC to generate something easier to handle (this is what we 
did for MIPS64).



>
>>
>>
>>
>>> This means on the second call to these subroutines they
>>> execute with only one nop and crash. I could  write  some new code
>>> that wrote the 2 nops at once, but (now that I understand
>>> "stop_machine") with the branch likely solution we should be able to
>>> stop using "stop_machine" when we write nops to the 20-30 thousand
>>> Linux functions. It looks like other platforms have stopped using
>>> stop_machine.
>>
>>
>> I don't particularly object to the 'branch likely solution', but I think the
>> failures of the other approaches indicates underlying bugs in the tracing
>> code.  Those bugs should probably be fixed.
>
> If a solution can be found that modifies a single 32bit instruction to
> enable/disable tracing, I don't see any bugs in the underlying code.
> Plus we can avoid using stop_machine().
>
>>
>> David Daney
>>
>>
>>
>>>
>>> Al
>>>
>>> On Fri, Jan 11, 2013 at 12:01 PM, David Daney <ddaney.cavm@gmail.com>
>>> wrote:
>>>>
>>>> On 01/11/2013 06:33 AM, Al Cooper wrote:
>>>>>
>>>>>
>>>>> Function tracing is currently broken for all 32 bit MIPS platforms.
>>>>> When tracing is enabled, the kernel immediately hangs on boot.
>>>>> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0
>>>>> that changes the kernel/trace/Kconfig file so that is no longer
>>>>> forces FRAME_POINTER when FUNCTION_TRACING is enabled.
>>>>>
>>>>> MIPS frame pointers are generally considered to be useless because
>>>>> they cannot be used to unwind the stack. Unfortunately the MIPS
>>>>> function tracing code has bugs that are masked by the use of frame
>>>>> pointers. This commit fixes the bugs so that MIPS frame pointers do
>>>>> not need to be enabled.
>>>>>
>>>>> The bugs are a result of the odd calling sequence used to call the trace
>>>>> routine. This calling sequence is inserted into every tracable function
>>>>> when the tracing CONFIG option is enabled. This sequence is generated
>>>>> for 32bit MIPS platforms by the compiler via the "-pg" flag.
>>>>> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
>>>>> call to the trace routine "_mcount" (some legacy thing where 2 arguments
>>>>> used to be pushed on the stack). The _mcount routine is expected to
>>>>> adjust the sp by +8 before returning.
>>>>>
>>>>> One of the bugs is that when tracing is disabled for a function, the
>>>>> "jalr _mcount" instruction is replaced with a nop, but the
>>>>> "addiu sp,sp,-8" is still executed and the stack pointer is left
>>>>> trashed. When frame pointers are enabled the problem is masked
>>>>> because any access to the stack is done through the frame
>>>>> pointer and the stack pointer is restored from the frame pointer when
>>>>> the function returns. This patch uses a branch likely instruction
>>>>> "bltzl zero, f1" instead of "nop" to disable the call because this
>>>>> instruction will not execute the "addiu sp,sp,-8" instruction in
>>>>> the delay slot. The other possible solution would be to nop out both
>>>>> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt
>>>>> and SMP safe and would be much more intrusive.
>>>>
>>>>
>>>>
>>>> I thought all CPUs were in stop_machine() when the modifications were
>>>> done,
>>>> so that there is no issue with multi-word instruction patching.
>>>>
>>>> Am I wrong about this?
>>>>
>>>> So really I think you can do two NOP just as easily.
>>>>
>>>> The only reason I bring this up is that I am not sure all 32-bit CPUs
>>>> implement the 'Likely' branch variants. Also there may be an affect on
>>>> the
>>>> branch predictor.
>>>>
>>>> A third possibility would be to replace the JALR with 'ADDIU SP,SP,8'
>>>> That
>>>> way the following adjustment would be canceled out.  This would work well
>>>> on
>>>> single-issue CPUs, but the instructions may not be able to dual-issue on
>>>> a
>>>> multi issue machine due to data dependencies.
>>>>
>>>> David Daney
>>>>
>>>>
>>>>>
>>>>> A few other bugs were fixed where the _mcount routine itself did not
>>>>> always fix the sp on return.
>>>>>
>>>>
>>>
>>>
>>
>
>

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Steven Rostedt]

On Fri, Jan 11, 2013 at 09:01:01AM -0800, David Daney wrote:
> 
> I thought all CPUs were in stop_machine() when the modifications
> were done, so that there is no issue with multi-word instruction
> patching.
> 
> Am I wrong about this?
> 
> So really I think you can do two NOP just as easily.

The problem with double NOPs is that it can only work if there's no
problem executing one nop and a non NOP. Which I think is an issue here.


If you have something like:

	bl	_mcount
	addiu	sp,sp,-8

And you convert that to:

	nop
	nop

Now if you convert that back to:

	bl	ftrace_caller
	addiu	sp,sp,-8

then you can have an issue if the task was preempted after that first
nop. Because stop_machine() doesn't wait for tasks to exit kernel space.
If you have a CONFIG_PREEMPT kernel, a task can be sleeping anywhere.
Thus you have a task execute the first nop, get preempted. You update
the code to be:

	bl	ftrace_caller
	addiu	sp,sp,-8

When that task gets scheduled back in, it will act like it just
executed:

	nop
	addiu	sp,sp,-8

Which is the problem you're trying to solve in the first place.

Now that said, There's no reason we need that addiu sp,sp,-8 there.
That's just what the mips defined mcount requires. But as you can see
above, with dynamic ftrace, the defined mcount is only called at boot
up, and never again. That means at boot up you can convert to:

	nop
	nop

and then when you enable tracing just convert it to:

	bl	ftrace_caller
	nop

There's nothing that states what the ftrace caller must be. We can have
it do a proper stack update. That is, only at boot up do we need to
handle the defined mcount. After that, those instructions are just place
holders for our own algorithms. If the addiu was needed for the defined
mcount, there's no reason to keep it for our own ftrace_caller.

Would that work?

-- Steve

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Alan Cooper]

On Mon, Jan 14, 2013 at 10:40 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, Jan 11, 2013 at 09:01:01AM -0800, David Daney wrote:
>>
>> I thought all CPUs were in stop_machine() when the modifications
>> were done, so that there is no issue with multi-word instruction
>> patching.
>>
>> Am I wrong about this?
>>
>> So really I think you can do two NOP just as easily.
>
> The problem with double NOPs is that it can only work if there's no
> problem executing one nop and a non NOP. Which I think is an issue here.
>
>
> If you have something like:
>
>         bl      _mcount
>         addiu   sp,sp,-8
>
> And you convert that to:
>
>         nop
>         nop
>
> Now if you convert that back to:
>
>         bl      ftrace_caller
>         addiu   sp,sp,-8
>
> then you can have an issue if the task was preempted after that first
> nop. Because stop_machine() doesn't wait for tasks to exit kernel space.
> If you have a CONFIG_PREEMPT kernel, a task can be sleeping anywhere.
> Thus you have a task execute the first nop, get preempted. You update
> the code to be:
>
>         bl      ftrace_caller
>         addiu   sp,sp,-8
>
> When that task gets scheduled back in, it will act like it just
> executed:
>
>         nop
>         addiu   sp,sp,-8
>
> Which is the problem you're trying to solve in the first place.
>
> Now that said, There's no reason we need that addiu sp,sp,-8 there.
> That's just what the mips defined mcount requires. But as you can see
> above, with dynamic ftrace, the defined mcount is only called at boot
> up, and never again. That means at boot up you can convert to:
>
>         nop
>         nop
>
> and then when you enable tracing just convert it to:
>
>         bl      ftrace_caller
>         nop
>
> There's nothing that states what the ftrace caller must be. We can have
> it do a proper stack update. That is, only at boot up do we need to
> handle the defined mcount. After that, those instructions are just place
> holders for our own algorithms. If the addiu was needed for the defined
> mcount, there's no reason to keep it for our own ftrace_caller.
>
> Would that work?
>
> -- Steve
>

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Steven Rostedt]

On Tue, 2013-01-15 at 12:53 -0500, Alan Cooper wrote:
> On Mon, Jan 14, 2013 at 10:40 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Fri, Jan 11, 2013 at 09:01:01AM -0800, David Daney wrote:
> >>
> >> I thought all CPUs were in stop_machine() when the modifications
> >> were done, so that there is no issue with multi-word instruction
> >> patching.
> >>
> >> Am I wrong about this?
> >>
> >> So really I think you can do two NOP just as easily.
> >
> > The problem with double NOPs is that it can only work if there's no
> > problem executing one nop and a non NOP. Which I think is an issue here.
> >
> >
> > If you have something like:
> >
> >         bl      _mcount
> >         addiu   sp,sp,-8
> >
> > And you convert that to:
> >
> >         nop
> >         nop
> >
> > Now if you convert that back to:
> >
> >         bl      ftrace_caller
> >         addiu   sp,sp,-8
> >
> > then you can have an issue if the task was preempted after that first
> > nop. Because stop_machine() doesn't wait for tasks to exit kernel space.
> > If you have a CONFIG_PREEMPT kernel, a task can be sleeping anywhere.
> > Thus you have a task execute the first nop, get preempted. You update
> > the code to be:
> >
> >         bl      ftrace_caller
> >         addiu   sp,sp,-8
> >
> > When that task gets scheduled back in, it will act like it just
> > executed:
> >
> >         nop
> >         addiu   sp,sp,-8
> >
> > Which is the problem you're trying to solve in the first place.
> >
> > Now that said, There's no reason we need that addiu sp,sp,-8 there.
> > That's just what the mips defined mcount requires. But as you can see
> > above, with dynamic ftrace, the defined mcount is only called at boot
> > up, and never again. That means at boot up you can convert to:
> >
> >         nop
> >         nop
> >
> > and then when you enable tracing just convert it to:
> >
> >         bl      ftrace_caller
> >         nop
> >
> > There's nothing that states what the ftrace caller must be. We can have
> > it do a proper stack update. That is, only at boot up do we need to
> > handle the defined mcount. After that, those instructions are just place
> > holders for our own algorithms. If the addiu was needed for the defined
> > mcount, there's no reason to keep it for our own ftrace_caller.
> >
> > Would that work?
> >
> > -- Steve
> >

Lost for words? :-)

-- Steve

Re: [PATCH] mips: function tracer: Fix broken function tracing

[David Daney]

On 01/14/2013 07:40 PM, Steven Rostedt wrote:
> On Fri, Jan 11, 2013 at 09:01:01AM -0800, David Daney wrote:
>>
>> I thought all CPUs were in stop_machine() when the modifications
>> were done, so that there is no issue with multi-word instruction
>> patching.
>>
>> Am I wrong about this?
>>
>> So really I think you can do two NOP just as easily.
>
> The problem with double NOPs is that it can only work if there's no
> problem executing one nop and a non NOP. Which I think is an issue here.
>
>
> If you have something like:
>
> 	bl	_mcount
> 	addiu	sp,sp,-8
>
> And you convert that to:
>
> 	nop
> 	nop
>
> Now if you convert that back to:
>
> 	bl	ftrace_caller
> 	addiu	sp,sp,-8
>
> then you can have an issue if the task was preempted after that first
> nop. Because stop_machine() doesn't wait for tasks to exit kernel space.
> If you have a CONFIG_PREEMPT kernel, a task can be sleeping anywhere.
> Thus you have a task execute the first nop, get preempted. You update
> the code to be:

Thanks for the explanation Steven.  This is the part I was missing.


Given all of this, I think the most expedient course for the short term 
is to use the branch-likely-false trick.  Although the performance will 
probably not be great, I think it is probably race free.

In the longer term...

>
> 	bl	ftrace_caller
> 	addiu	sp,sp,-8
>
> When that task gets scheduled back in, it will act like it just
> executed:
>
> 	nop
> 	addiu	sp,sp,-8
>
> Which is the problem you're trying to solve in the first place.
>
> Now that said, There's no reason we need that addiu sp,sp,-8 there.
> That's just what the mips defined mcount requires. But as you can see
> above, with dynamic ftrace, the defined mcount is only called at boot
> up, and never again. That means at boot up you can convert to:
>
> 	nop
> 	nop
>
> and then when you enable tracing just convert it to:
>
> 	bl	ftrace_caller
> 	nop
>
> There's nothing that states what the ftrace caller must be. We can have
> it do a proper stack update. That is, only at boot up do we need to
> handle the defined mcount. After that, those instructions are just place
> holders for our own algorithms. If the addiu was needed for the defined
> mcount, there's no reason to keep it for our own ftrace_caller.
>
> Would that work?

... either do as you suggest and dynamically change the ABI of the 
target function.

Or add support to GCC for a better tracing ABI (as I already said we did 
for mips64).


Thanks,
David Daney

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Steven Rostedt]

On Tue, 2013-01-15 at 09:55 -0800, David Daney wrote:

> > There's nothing that states what the ftrace caller must be. We can have
> > it do a proper stack update. That is, only at boot up do we need to
> > handle the defined mcount. After that, those instructions are just place
> > holders for our own algorithms. If the addiu was needed for the defined
> > mcount, there's no reason to keep it for our own ftrace_caller.
> >
> > Would that work?
> 
> ... either do as you suggest and dynamically change the ABI of the 
> target function.

We already change the ABI. We have it call ftrace_caller instead of
mcount.

BTW, I've just compiled with gcc 4.6.3 against mips, and I don't see the
issue. I have:

0000000000000000 <account_kernel_stack>:
       0:       03e0082d        move    at,ra
       4:       0c000000        jal     0 <account_kernel_stack>
                        4: R_MIPS_26    _mcount
                        4: R_MIPS_NONE  *ABS*
                        4: R_MIPS_NONE  *ABS*
       8:       0000602d        move    t0,zero
       c:       2402000d        li      v0,13
      10:       3c030000        lui     v1,0x0
                        10: R_MIPS_HI16 mem_section
                        10: R_MIPS_NONE *ABS*
                        10: R_MIPS_NONE *ABS*
      14:       000216fc        dsll32  v0,v0,0x1b
      18:       64630000        daddiu  v1,v1,0

Is it dependent on the config?

> 
> Or add support to GCC for a better tracing ABI (as I already said we did 
> for mips64).

I wouldn't waste time changing gcc for this. If you're going to change
gcc than please implement the -mfentry option. Look at x86_64 to
understand this more.

-- Steve

Re: [PATCH] mips: function tracer: Fix broken function tracing

[David Daney]

On 01/15/2013 01:07 PM, Steven Rostedt wrote:
> On Tue, 2013-01-15 at 09:55 -0800, David Daney wrote:
>
>>> There's nothing that states what the ftrace caller must be. We can have
>>> it do a proper stack update. That is, only at boot up do we need to
>>> handle the defined mcount. After that, those instructions are just place
>>> holders for our own algorithms. If the addiu was needed for the defined
>>> mcount, there's no reason to keep it for our own ftrace_caller.
>>>
>>> Would that work?
>>
>> ... either do as you suggest and dynamically change the ABI of the
>> target function.
>
> We already change the ABI. We have it call ftrace_caller instead of
> mcount.
>
> BTW, I've just compiled with gcc 4.6.3 against mips, and I don't see the
> issue. I have:
>
> 0000000000000000 <account_kernel_stack>:
>         0:       03e0082d        move    at,ra
>         4:       0c000000        jal     0 <account_kernel_stack>
>                          4: R_MIPS_26    _mcount
>                          4: R_MIPS_NONE  *ABS*
>                          4: R_MIPS_NONE  *ABS*
>         8:       0000602d        move    t0,zero
>         c:       2402000d        li      v0,13
>        10:       3c030000        lui     v1,0x0
>                          10: R_MIPS_HI16 mem_section
>                          10: R_MIPS_NONE *ABS*
>                          10: R_MIPS_NONE *ABS*
>        14:       000216fc        dsll32  v0,v0,0x1b
>        18:       64630000        daddiu  v1,v1,0
>
> Is it dependent on the config?

Yes.

You need to select a 32-bit kernel (which in turn may require selecting 
a board type that also supports it).

The ABI is different for 32-bit and 64-bit _mcount.

David Daney


>
>>
>> Or add support to GCC for a better tracing ABI (as I already said we did
>> for mips64).
>
> I wouldn't waste time changing gcc for this. If you're going to change
> gcc than please implement the -mfentry option. Look at x86_64 to
> understand this more.

A good point.  But I don't really plan on doing any work related to 
32-bit mips things at this point, so any such change would have to be 
done by someone else.

David Daney

Re: [PATCH] mips: function tracer: Fix broken function tracing

[Alan Cooper]

On Tue, Jan 15, 2013 at 4:34 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 01/15/2013 01:07 PM, Steven Rostedt wrote:
>>
>> On Tue, 2013-01-15 at 09:55 -0800, David Daney wrote:
>>
>>>> There's nothing that states what the ftrace caller must be. We can have
>>>> it do a proper stack update. That is, only at boot up do we need to
>>>> handle the defined mcount. After that, those instructions are just place
>>>> holders for our own algorithms. If the addiu was needed for the defined
>>>> mcount, there's no reason to keep it for our own ftrace_caller.
>>>>
>>>> Would that work?
>>>
>>>
>>> ... either do as you suggest and dynamically change the ABI of the
>>> target function.
>>
>>
>> We already change the ABI. We have it call ftrace_caller instead of
>> mcount.
>>
>> BTW, I've just compiled with gcc 4.6.3 against mips, and I don't see the
>> issue. I have:
>>
>> 0000000000000000 <account_kernel_stack>:
>>         0:       03e0082d        move    at,ra
>>         4:       0c000000        jal     0 <account_kernel_stack>
>>                          4: R_MIPS_26    _mcount
>>                          4: R_MIPS_NONE  *ABS*
>>                          4: R_MIPS_NONE  *ABS*
>>         8:       0000602d        move    t0,zero
>>         c:       2402000d        li      v0,13
>>        10:       3c030000        lui     v1,0x0
>>                          10: R_MIPS_HI16 mem_section
>>                          10: R_MIPS_NONE *ABS*
>>                          10: R_MIPS_NONE *ABS*
>>        14:       000216fc        dsll32  v0,v0,0x1b
>>        18:       64630000        daddiu  v1,v1,0
>>
>> Is it dependent on the config?
>
>
> Yes.
>
> You need to select a 32-bit kernel (which in turn may require selecting a
> board type that also supports it).
>
> The ABI is different for 32-bit and 64-bit _mcount.
>
> David Daney
>

Building for MIPS malta will show the problem.

>
>
>>
>>>
>>> Or add support to GCC for a better tracing ABI (as I already said we did
>>> for mips64).
>>
>>
>> I wouldn't waste time changing gcc for this. If you're going to change
>> gcc than please implement the -mfentry option. Look at x86_64 to
>> understand this more.
>
>
> A good point.  But I don't really plan on doing any work related to 32-bit
> mips things at this point, so any such change would have to be done by
> someone else.
>
> David Daney
>

I love the idea of removing the useless stack adjust stuff at run time!
The issue still remains for the initial writing of the 2 nops. It
looks like the initial call to write the nops is done from ftrace_init
which is called before SMP is up, so if I write the 2 nops via a
single call to a function with interrupts disabled it should be safe.
I also need to do this for modules at insmod time.

This has been GREAT feedback!

Thanks