[PATCH 0/2] Minor MIPS ftrace fixes

[PATCH 0/2] Minor MIPS ftrace fixes

[Markos Chandras]

Hi,

A few more fixes for ftrace/MIPS. The first patch fixes the
value of the MCOUNT_INSN_SIZE definition which holds the
total size of the mcount() call. The second one, fixes
the selfpc argument for the ftrace tracing function.

Markos Chandras (2):
  MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition
  MIPS: mcount: Fix selfpc address for static trace

 arch/mips/include/asm/ftrace.h | 2 +-
 arch/mips/kernel/ftrace.c      | 4 +++-
 arch/mips/kernel/mcount.S      | 4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.1.0

[PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition

[Markos Chandras]

The MCOUNT_INSN_SIZE is meant to be used to denote the overall
size of the mcount() call. Since a jal instruction is used to
call mcount() the delay slot should be taken into consideration
as well.
This also replaces the MCOUNT_INSN_SIZE usage with the real size
of a single MIPS instruction since, as described above, the
MCOUNT_INSN_SIZE is used to denote the total overhead of the
mcount() call.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/ftrace.h | 2 +-
 arch/mips/kernel/ftrace.c      | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/ftrace.h b/arch/mips/include/asm/ftrace.h
index 992aaba603b5..70d4a35fb560 100644
--- a/arch/mips/include/asm/ftrace.h
+++ b/arch/mips/include/asm/ftrace.h
@@ -13,7 +13,7 @@
 #ifdef CONFIG_FUNCTION_TRACER
 
 #define MCOUNT_ADDR ((unsigned long)(_mcount))
-#define MCOUNT_INSN_SIZE 4		/* sizeof mcount call */
+#define MCOUNT_INSN_SIZE 8		/* sizeof mcount call + delay slot */
 
 #ifndef __ASSEMBLY__
 extern void _mcount(void);
diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 937c54bc8ccc..211460d4617d 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -28,6 +28,8 @@
 #define MCOUNT_OFFSET_INSNS 4
 #endif
 
+#define FTRACE_MIPS_INSN_SIZE 4 /* Size of single MIPS instruction */
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 /* Arch override because MIPS doesn't need to run this from stop_machine() */
@@ -395,7 +397,7 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 	 */
 
 	insns = in_kernel_space(self_ra) ? 2 : MCOUNT_OFFSET_INSNS + 1;
-	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
+	trace.func = self_ra - (FTRACE_MIPS_INSN_SIZE * insns);
 
 	/* Only trace if the calling function expects to */
 	if (!ftrace_graph_entry(&trace)) {
-- 
2.1.0

[PATCH 2/2] MIPS: mcount: Fix selfpc address for static trace

[Markos Chandras]

According to Documentation/trace/ftrace-design.txt, the selfpc
should be the return address minus the mcount overhead (8 bytes).
This brings static trace in line with the dynamic trace regarding
the selfpc argument to the tracing function.

This also removes the magic number '8' with the proper
MCOUNT_INSN_SIZE.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/mcount.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
index 2f7c734771f4..3af48b7c7a47 100644
--- a/arch/mips/kernel/mcount.S
+++ b/arch/mips/kernel/mcount.S
@@ -79,7 +79,7 @@ _mcount:
 	PTR_S	MCOUNT_RA_ADDRESS_REG, PT_R12(sp)
 #endif
 
-	PTR_SUBU a0, ra, 8	/* arg1: self address */
+	PTR_SUBU a0, ra, MCOUNT_INSN_SIZE /* arg1: self address */
 	PTR_LA   t1, _stext
 	sltu     t2, a0, t1	/* t2 = (a0 < _stext) */
 	PTR_LA   t1, _etext
@@ -138,7 +138,7 @@ NESTED(_mcount, PT_SIZE, ra)
 static_trace:
 	MCOUNT_SAVE_REGS
 
-	move	a0, ra		/* arg1: self return address */
+	PTR_SUBU a0, ra, MCOUNT_INSN_SIZE	/* arg1: self address */
 	jalr	t2		/* (1) call *ftrace_trace_function */
 	 move	a1, AT		/* arg2: parent's return address */
 
-- 
2.1.0

Re: [PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition

[David Daney]

On 09/22/2014 06:32 AM, Markos Chandras wrote:
> The MCOUNT_INSN_SIZE is meant to be used to denote the overall
> size of the mcount() call. Since a jal instruction is used to
> call mcount() the delay slot should be taken into consideration
> as well.
> This also replaces the MCOUNT_INSN_SIZE usage with the real size
> of a single MIPS instruction since, as described above, the
> MCOUNT_INSN_SIZE is used to denote the total overhead of the
> mcount() call.

Are you seeing errors with the existing code?  If so please state what 
they are.

By changing this, we can no longer atomically replace the instruction. 
So I think shouldn't be changing this stuff unless there is a real bug 
we are fixing.

In conclusion: NAK unless the patch fixes a bug, in which case the 
change log *must* state what the bug is, and how the patch addresses the 
problem.

David Daney


>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>   arch/mips/include/asm/ftrace.h | 2 +-
>   arch/mips/kernel/ftrace.c      | 4 +++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/ftrace.h b/arch/mips/include/asm/ftrace.h
> index 992aaba603b5..70d4a35fb560 100644
> --- a/arch/mips/include/asm/ftrace.h
> +++ b/arch/mips/include/asm/ftrace.h
> @@ -13,7 +13,7 @@
>   #ifdef CONFIG_FUNCTION_TRACER
>
>   #define MCOUNT_ADDR ((unsigned long)(_mcount))
> -#define MCOUNT_INSN_SIZE 4		/* sizeof mcount call */
> +#define MCOUNT_INSN_SIZE 8		/* sizeof mcount call + delay slot */
>
>   #ifndef __ASSEMBLY__
>   extern void _mcount(void);
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 937c54bc8ccc..211460d4617d 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -28,6 +28,8 @@
>   #define MCOUNT_OFFSET_INSNS 4
>   #endif
>
> +#define FTRACE_MIPS_INSN_SIZE 4 /* Size of single MIPS instruction */
> +
>   #ifdef CONFIG_DYNAMIC_FTRACE
>
>   /* Arch override because MIPS doesn't need to run this from stop_machine() */
> @@ -395,7 +397,7 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
>   	 */
>
>   	insns = in_kernel_space(self_ra) ? 2 : MCOUNT_OFFSET_INSNS + 1;
> -	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
> +	trace.func = self_ra - (FTRACE_MIPS_INSN_SIZE * insns);
>
>   	/* Only trace if the calling function expects to */
>   	if (!ftrace_graph_entry(&trace)) {
>

Re: [PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition

[Steven Rostedt]

On Mon, 22 Sep 2014 09:55:09 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

> On 09/22/2014 06:32 AM, Markos Chandras wrote:
> > The MCOUNT_INSN_SIZE is meant to be used to denote the overall
> > size of the mcount() call. Since a jal instruction is used to
> > call mcount() the delay slot should be taken into consideration
> > as well.
> > This also replaces the MCOUNT_INSN_SIZE usage with the real size
> > of a single MIPS instruction since, as described above, the
> > MCOUNT_INSN_SIZE is used to denote the total overhead of the
> > mcount() call.
> 
> Are you seeing errors with the existing code?  If so please state what 
> they are.
> 
> By changing this, we can no longer atomically replace the instruction. 
> So I think shouldn't be changing this stuff unless there is a real bug 
> we are fixing.

Actually, it looks like the code still works the same, as it uses the
old size of 4 (FTRACE_MIPS_INSN_SIZE) to do the update.

> 
> In conclusion: NAK unless the patch fixes a bug, in which case the 
> change log *must* state what the bug is, and how the patch addresses the 
> problem.
> 

I agree that the change log needs to explicitly state what is being
fixed. The only thing I can think of is if MIPS has kprobes, and
kprobes does different logic or may reject completely changes to ftrace
mcount call locations. This change will have kprobes flag the delay
slot as owned by ftrace.

It may also fix the stack tracer, as it searches for the ip saved in
the return address to find where the true stack is (skipping the stack
part that calls the strack tracer itself). If the link register holds
the location after the delay slot, then this would require
MCOUNT_INSN_SIZE to include the delay slot as well. Or I could add
another macro called MCOUNT_DELAY_SLOT_SIZE that can be defined by an
arch (and keep it zero for all other archs). That wouldn't be too much
of an issue to implement.

But again, the change log needs to express what is being fixed, which I
don't see.

I'm willing to update the generic code to express different ways of
implementation to keep the archs from doing hacks like this.

-- Steve

Re: [PATCH 2/2] MIPS: mcount: Fix selfpc address for static trace

[Steven Rostedt]

On Mon, 22 Sep 2014 14:32:59 +0100
Markos Chandras <markos.chandras@imgtec.com> wrote:

> According to Documentation/trace/ftrace-design.txt, the selfpc
> should be the return address minus the mcount overhead (8 bytes).
> This brings static trace in line with the dynamic trace regarding
> the selfpc argument to the tracing function.
> 
> This also removes the magic number '8' with the proper
> MCOUNT_INSN_SIZE.

I could also update the generic code to handle delay slots.

-- Steve

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>  arch/mips/kernel/mcount.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
> index 2f7c734771f4..3af48b7c7a47 100644
> --- a/arch/mips/kernel/mcount.S
> +++ b/arch/mips/kernel/mcount.S
> @@ -79,7 +79,7 @@ _mcount:
>  	PTR_S	MCOUNT_RA_ADDRESS_REG, PT_R12(sp)
>  #endif
>  
> -	PTR_SUBU a0, ra, 8	/* arg1: self address */
> +	PTR_SUBU a0, ra, MCOUNT_INSN_SIZE /* arg1: self address */
>  	PTR_LA   t1, _stext
>  	sltu     t2, a0, t1	/* t2 = (a0 < _stext) */
>  	PTR_LA   t1, _etext
> @@ -138,7 +138,7 @@ NESTED(_mcount, PT_SIZE, ra)
>  static_trace:
>  	MCOUNT_SAVE_REGS
>  
> -	move	a0, ra		/* arg1: self return address */
> +	PTR_SUBU a0, ra, MCOUNT_INSN_SIZE	/* arg1: self address */
>  	jalr	t2		/* (1) call *ftrace_trace_function */
>  	 move	a1, AT		/* arg2: parent's return address */
>  

Re: [PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition

[Markos Chandras]

On 09/22/2014 07:25 PM, Steven Rostedt wrote:
> On Mon, 22 Sep 2014 09:55:09 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
> 
>> On 09/22/2014 06:32 AM, Markos Chandras wrote:
>>> The MCOUNT_INSN_SIZE is meant to be used to denote the overall
>>> size of the mcount() call. Since a jal instruction is used to
>>> call mcount() the delay slot should be taken into consideration
>>> as well.
>>> This also replaces the MCOUNT_INSN_SIZE usage with the real size
>>> of a single MIPS instruction since, as described above, the
>>> MCOUNT_INSN_SIZE is used to denote the total overhead of the
>>> mcount() call.
>>
>> Are you seeing errors with the existing code?  If so please state what 
>> they are.
>>
>> By changing this, we can no longer atomically replace the instruction. 
>> So I think shouldn't be changing this stuff unless there is a real bug 
>> we are fixing.
> 
> Actually, it looks like the code still works the same, as it uses the
> old size of 4 (FTRACE_MIPS_INSN_SIZE) to do the update.

Indeed I haven't seen any functional change when it comes to replacing
the instruction.

> [...]
> 
> It may also fix the stack tracer, as it searches for the ip saved in
> the return address to find where the true stack is (skipping the stack
> part that calls the strack tracer itself). If the link register holds
> the location after the delay slot, then this would require
> MCOUNT_INSN_SIZE to include the delay slot as well.

Yes, this is the only case I spotted as well. Perhaps I should put that
in the changelog.

Or I could add
> another macro called MCOUNT_DELAY_SLOT_SIZE that can be defined by an
> arch (and keep it zero for all other archs). That wouldn't be too much
> of an issue to implement.

If you want to fix that in the generic code then I am fine with it.

-- 
markos

Re: [PATCH 2/2] MIPS: mcount: Fix selfpc address for static trace

[Markos Chandras]

On 09/22/2014 07:26 PM, Steven Rostedt wrote:
> On Mon, 22 Sep 2014 14:32:59 +0100
> Markos Chandras <markos.chandras@imgtec.com> wrote:
> 
>> According to Documentation/trace/ftrace-design.txt, the selfpc
>> should be the return address minus the mcount overhead (8 bytes).
>> This brings static trace in line with the dynamic trace regarding
>> the selfpc argument to the tracing function.
>>
>> This also removes the magic number '8' with the proper
>> MCOUNT_INSN_SIZE.
> 
> I could also update the generic code to handle delay slots.
> 
> -- Steve

As I said to the other patch, if you want to fix the delay slots in the
generic code that may be preferred indeed. On the other hand, the static
tracer still needs fixing so the correct selfpc is used. I will update
this patch based on the way you choose to handle delay slots in the
generic code. Thanks

-- 
markos