[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(-)

[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 <***@goodmis.org>
Cc: linux-***@vger.kernel.org
Signed-off-by: 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 */

[Steven Rostedt]

On Mon, 22 Sep 2014 14:32:59 +0100

Post by 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.

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

-- Steve

Post by Markos Chandras
---
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
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)
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 */

[Markos Chandras]

Post by Steven Rostedt
On Mon, 22 Sep 2014 14:32:59 +0100

Post by 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.

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 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 <***@goodmis.org>
Cc: Ingo Molnar <***@redhat.com>
Cc: linux-***@vger.kernel.org
Signed-off-by: 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)) {

[David Daney]

Post by 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.

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

Post by Markos Chandras
---
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)) {

[Steven Rostedt]

On Mon, 22 Sep 2014 09:55:09 -0700

Post by David Daney

Post by 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.

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.

Post by David Daney
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

[Markos Chandras]

Post by Steven Rostedt
On Mon, 22 Sep 2014 09:55:09 -0700

Post by David Daney

Post by 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.

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.

Post by Steven Rostedt
[...]
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

Post by Steven Rostedt
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.