Skip to contentSkip to conversation

tracing: MIPS: add misc fixups and cleanups

PATCH Superseded Wu Zhangjin 3 participants 16 messages May 12, 2010 – May 13, 2010

Subsystems (2)

v6 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v6 cover tracing: MIPS: add misc fixups and cleanups

From: Wu Zhangjin <[email protected]>

This patchset adds misc fixups and cleanups for Ftrace of MIPS:

+ Fix the support of 32bit support with -mmcount-ra-address of gcc 4.5
o tracing: MIPS: mcount.S: Fixup of the 32bit support with gcc 4.5 The argument is passed by $12 in 32bit, not t0.
o tracing: MIPS: Fixup of the 32bit support with -mmcount-ra-address For 32bit kernel, the offset of "b 1f" should be 5 instructions, not only 4.

+ Speedup the dynamic function tracer
o tracing: MIPS: Reduce the overhead of dynamic Function Tracer In the old implementation, we have encode the 'nop' instruction and the instruction of calling to mcount at run-time, which may add some overhead. We reduce this overhead via encoding them when initializing the dynamic function tracer.

+ Lots of cleanups
o The other patches.

----------------

Changes from v5:

o splits up the old v5 revision into several patches to make the maintainer happier to review it.

Regards,
Wu Zhangjin

Wu Zhangjin (9):
tracing: MIPS: mcount.S: merge the same continuous #ifdefs
tracing: MIPS: mcount.S: Fixup of the 32bit support with gcc 4.5
tracing: MIPS: mcount.S: cleanup the arguments of
prepare_ftrace_return
tracing: MIPS: mcount.S: cleanup of the comments
tracing: MIPS: Fixup of the 32bit support with -mmcount-ra-address
tracing: MIPS: cleanup of the instructions
tracing: MIPS: Reduce the overhead of dynamic Function Tracer
tracing: MIPS: cleanup of function graph tracer
tracing: MIPS: cleanup of the address space checking

 arch/mips/kernel/ftrace.c |  179 +++++++++++++++++++++++++++------------------
 arch/mips/kernel/mcount.S |   49 +++++++-----
 2 files changed, 135 insertions(+), 93 deletions(-)
#1 lore raw

PATCH v6 1/9 tracing: MIPS: mcount.S: merge the same continuous #ifdefs

From: Wu Zhangjin <[email protected]>

There are two same continuous #ifdefs, this patch merges them to reduce two lines.

---
 arch/mips/kernel/mcount.S |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
arch/mips/kernel/mcount.S +0 -2
index 6851fc9..e256bf9 100644
@@ -45,8 +45,6 @@
 	PTR_L	a5, PT_R9(sp)
 	PTR_L	a6, PT_R10(sp)
 	PTR_L	a7, PT_R11(sp)
-#endif
-#ifdef CONFIG_64BIT
 	PTR_ADDIU	sp, PT_SIZE
 #else
 	PTR_ADDIU	sp, (PT_SIZE + 8)
Signature · 1 line
1.7.0.4
#2 lore raw

PATCH v6 2/9 tracing: MIPS: mcount.S: Fixup of the 32bit support with gcc 4.5

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

As the doc[1] of gcc-4.5 shows, the -mmcount-ra-address uses register $12 to transfer the stack offset of the return address to the _mcount function. in 64bit kernel, $12 is t0, but in 32bit kernel, it is t4, so, we need to use $12 instead of t0 here to cover the 64bit and 32bit support.

[1] Gcc doc: MIPS Options
http://gcc.gnu.org/onlinedocs/gcc/MIPS-Options.html

---
 arch/mips/kernel/mcount.S |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
arch/mips/kernel/mcount.S +5 -5
index e256bf9..92d1540 100644
@@ -69,7 +69,7 @@ _mcount:
 
 	MCOUNT_SAVE_REGS
 #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	PTR_S	t0, PT_R12(sp)	/* t0 saved the location of the return address(at) by -mmcount-ra-address */
+	PTR_S	$12, PT_R12(sp)	/* $12 saved the location of the return address(at) by -mmcount-ra-address */
 #endif
 
 	move	a0, ra		/* arg1: next ip, selfaddr */
@@ -135,7 +135,7 @@ NESTED(ftrace_graph_caller, PT_SIZE, ra)
 #ifdef CONFIG_DYNAMIC_FTRACE
 	PTR_L	a1, PT_R31(sp)	/* load the original ra from the stack */
 #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	PTR_L	t0, PT_R12(sp)	/* load the original t0 from the stack */
+	PTR_L	$12, PT_R12(sp)	/* load the original $12 from the stack */
 #endif
 #else
 	MCOUNT_SAVE_REGS
@@ -143,10 +143,10 @@ NESTED(ftrace_graph_caller, PT_SIZE, ra)
 #endif
 
 #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	bnez	t0, 1f		/* non-leaf func: t0 saved the location of the return address */
+	bnez	$12, 1f		/* non-leaf func: $12 saved the location of the return address */
 	 nop
-	PTR_LA	t0, PT_R1(sp)	/* leaf func: get the location of at(old ra) from our own stack */
-1:	move	a0, t0		/* arg1: the location of the return address */
+	PTR_LA	$12, PT_R1(sp)	/* leaf func: get the location of at(old ra) from our own stack */
+1:	move	a0, $12		/* arg1: the location of the return address */
 #else
 	PTR_LA	a0, PT_R1(sp)	/* arg1: &AT -> a0 */
 #endif
Signature · 1 line
1.7.0.4
#3 lore raw

Re: [PATCH 2/9] tracing: MIPS: mcount.S: Fixup of the 32bit support with gcc 4.5

Quoted 8 lines · Wu Zhangjin

From: Wu Zhangjin<[email protected]>

As the doc[1] of gcc-4.5 shows, the -mmcount-ra-address uses register $12 to transfer the stack offset of the return address to the _mcount function. in 64bit kernel, $12 is t0, but in 32bit kernel, it is t4, so, we need to use $12 instead of t0 here to cover the 64bit and 32bit support.

Quoted 4 lines

[1] Gcc doc: MIPS Options
http://gcc.gnu.org/onlinedocs/gcc/MIPS-Options.html

Would it be better to do?:

#define MCOUNT_RA_ADDRESS_REG $12

s/t0/MCOUNT_RA_ADDRESS_REG/g

David Daney

Quoted 40 lines
---
  arch/mips/kernel/mcount.S |   10 +++++-----
  1 files changed, 5 insertions(+), 5 deletions(-)
arch/mips/kernel/mcount.S +5 -5
index e256bf9..92d1540 100644
@@ -69,7 +69,7 @@ _mcount:
 
  	MCOUNT_SAVE_REGS
  #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	PTR_S	t0, PT_R12(sp)	/* t0 saved the location of the return address(at) by -mmcount-ra-address */
+	PTR_S	$12, PT_R12(sp)	/* $12 saved the location of the return address(at) by -mmcount-ra-address */
  #endif
 
  	move	a0, ra		/* arg1: next ip, selfaddr */
@@ -135,7 +135,7 @@ NESTED(ftrace_graph_caller, PT_SIZE, ra)
  #ifdef CONFIG_DYNAMIC_FTRACE
  	PTR_L	a1, PT_R31(sp)	/* load the original ra from the stack */
  #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	PTR_L	t0, PT_R12(sp)	/* load the original t0 from the stack */
+	PTR_L	$12, PT_R12(sp)	/* load the original $12 from the stack */
  #endif
  #else
  	MCOUNT_SAVE_REGS
@@ -143,10 +143,10 @@ NESTED(ftrace_graph_caller, PT_SIZE, ra)
  #endif
 
  #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	bnez	t0, 1f		/* non-leaf func: t0 saved the location of the return address */
+	bnez	$12, 1f		/* non-leaf func: $12 saved the location of the return address */
  	 nop
-	PTR_LA	t0, PT_R1(sp)	/* leaf func: get the location of at(old ra) from our own stack */
-1:	move	a0, t0		/* arg1: the location of the return address */
+	PTR_LA	$12, PT_R1(sp)	/* leaf func: get the location of at(old ra) from our own stack */
+1:	move	a0, $12		/* arg1: the location of the return address */
  #else
  	PTR_LA	a0, PT_R1(sp)	/* arg1:&AT ->  a0 */
  #endif
#4 lore raw

Re: [PATCH 2/9] tracing: MIPS: mcount.S: Fixup of the 32bit support with gcc 4.5

Quoted 12 lines · David Daney
Quoted 11 lines · Wu Zhangjin

From: Wu Zhangjin<[email protected]>

As the doc[1] of gcc-4.5 shows, the -mmcount-ra-address uses register $12 to transfer the stack offset of the return address to the _mcount function. in 64bit kernel, $12 is t0, but in 32bit kernel, it is t4, so, we need to use $12 instead of t0 here to cover the 64bit and 32bit support.

[1] Gcc doc: MIPS Options
http://gcc.gnu.org/onlinedocs/gcc/MIPS-Options.html

Quoted 7 lines
Quoted 1 line

Would it be better to do?:

#define MCOUNT_RA_ADDRESS_REG $12

s/t0/MCOUNT_RA_ADDRESS_REG/g

Good idea, will change it later.

Regards,
Wu Zhangjin

#5 lore raw

PATCH v6 3/9 tracing: MIPS: mcount.S: cleanup the arguments of prepare_ftrace_return

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

The old arguments handling for prepare_ftrace_return is awlful, this patch cleans it up.

---
 arch/mips/kernel/mcount.S |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)
arch/mips/kernel/mcount.S +22 -14
index 92d1540..991cbdf 100644
@@ -132,28 +132,34 @@ ftrace_stub:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
 NESTED(ftrace_graph_caller, PT_SIZE, ra)
-#ifdef CONFIG_DYNAMIC_FTRACE
-	PTR_L	a1, PT_R31(sp)	/* load the original ra from the stack */
-#ifdef KBUILD_MCOUNT_RA_ADDRESS
-	PTR_L	$12, PT_R12(sp)	/* load the original $12 from the stack */
-#endif
-#else
+#ifndef CONFIG_DYNAMIC_FTRACE
 	MCOUNT_SAVE_REGS
-	move	a1, ra		/* arg2: next ip, selfaddr */
 #endif
 
+	/* arg1: Get the location of the parent's return address */
 #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	bnez	$12, 1f		/* non-leaf func: $12 saved the location of the return address */
+#ifdef CONFIG_DYNAMIC_FTRACE
+	PTR_L	a0, PT_R12(sp)
+#else
+	move	a0, $12
+#endif
+	bnez	a0, 1f		/* non-leaf func: stored in $12 */
 	 nop
-	PTR_LA	$12, PT_R1(sp)	/* leaf func: get the location of at(old ra) from our own stack */
-1:	move	a0, $12		/* arg1: the location of the return address */
+#endif
+	PTR_LA	a0, PT_R1(sp)	/* leaf func: the location in current stack */
+1:
+
+	/* arg2: Get self return address */
+#ifdef CONFIG_DYNAMIC_FTRACE
+	PTR_L	a1, PT_R31(sp)
 #else
-	PTR_LA	a0, PT_R1(sp)	/* arg1: &AT -> a0 */
+	move	a1, ra
 #endif
-	jal	prepare_ftrace_return
+
+	/* arg3: Get frame pointer of current stack */
 #ifdef CONFIG_FRAME_POINTER
-	 move	a2, fp		/* arg3: frame pointer */
-#else
+	 move	a2, fp
+#else /* ! CONFIG_FRAME_POINTER */
 #ifdef CONFIG_64BIT
 	 PTR_LA	a2, PT_SIZE(sp)
 #else
@@ -161,6 +167,8 @@ NESTED(ftrace_graph_caller, PT_SIZE, ra)
 #endif
 #endif
 
+	jal	prepare_ftrace_return
+	 nop
 	MCOUNT_RESTORE_REGS
 	RETURN_BACK
 	END(ftrace_graph_caller)
Signature · 1 line
1.7.0.4
#6 lore raw

PATCH v6 4/9 tracing: MIPS: mcount.S: cleanup of the comments

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

This patch cleans the comments up.

---
 arch/mips/kernel/mcount.S |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
arch/mips/kernel/mcount.S +6 -5
index 991cbdf..84303bf 100644
@@ -6,6 +6,7 @@
  * more details.
  *
  * Copyright (C) 2009 Lemote Inc. & DSLab, Lanzhou University, China
+ * Copyright (C) 2010 DSLab, Lanzhou University, China
  * Author: Wu Zhangjin <[email protected]>
  */
 
@@ -69,14 +70,14 @@ _mcount:
 
 	MCOUNT_SAVE_REGS
 #ifdef KBUILD_MCOUNT_RA_ADDRESS
-	PTR_S	$12, PT_R12(sp)	/* $12 saved the location of the return address(at) by -mmcount-ra-address */
+	PTR_S	$12, PT_R12(sp)	/* save location of parent's return address */
 #endif
 
-	move	a0, ra		/* arg1: next ip, selfaddr */
+	move	a0, ra		/* arg1: self return address */
 	.globl ftrace_call
 ftrace_call:
 	nop	/* a placeholder for the call to a real tracing function */
-	 move	a1, AT		/* arg2: the caller's next ip, parent */
+	 move	a1, AT		/* arg2: parent's return address */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	.globl ftrace_graph_call
@@ -117,9 +118,9 @@ NESTED(_mcount, PT_SIZE, ra)
 static_trace:
 	MCOUNT_SAVE_REGS
 
-	move	a0, ra		/* arg1: next ip, selfaddr */
+	move	a0, ra		/* arg1: self return address */
 	jalr	t2		/* (1) call *ftrace_trace_function */
-	 move	a1, AT		/* arg2: the caller's next ip, parent */
+	 move	a1, AT		/* arg2: parent's return address */
 
 	MCOUNT_RESTORE_REGS
 	.globl ftrace_stub
Signature · 1 line
1.7.0.4
#7 lore raw

PATCH v6 5/9 tracing: MIPS: Fixup of the 32bit support with -mmcount-ra-address

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

For 32bit kernel, the -mmcount-ra-address option of gcc 4.5 has introduced one more instruction before calling to _mcount, so we need to use a different "b 1f" for it.

---
 arch/mips/kernel/ftrace.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)
arch/mips/kernel/ftrace.c +14 -2
index e9e64e0..37aa767 100644
@@ -62,14 +62,26 @@ int ftrace_make_nop(struct module *mod,
 				return -EFAULT;
 		}
 
+#if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
+		/* lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)
+		 * addiu v1, v1, low_16bit_of_mcount
+		 * move at, ra
+		 * move $12, ra_address
+		 * jalr v1
+		 *  sub sp, sp, 8
+		 *                                  1: offset = 5 instructions
+		 */
+		new = 0x10000005;
+#else
 		/* lui v1, hi_16bit_of_mcount        --> b 1f (0x10000004)
 		 * addiu v1, v1, low_16bit_of_mcount
 		 * move at, ra
 		 * jalr v1
-		 * nop
-		 * 				     1f: (ip + 12)
+		 *  nop | move $12, ra_address | sub sp, sp, 8
+		 *                                  1: offset = 4 instructions
 		 */
 		new = 0x10000004;
+#endif
 	} else {
 		/* record/calculate it for ftrace_make_call */
 		if (jal_mcount == 0) {
Signature · 1 line
1.7.0.4
#8 lore raw

PATCH v6 6/9 tracing: MIPS: cleanup of the instructions

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

This patch adds some cleanups of the instructions:
o use macro instead of magic numbers
o use macro instead of variables to reduce some overhead
o add a new macro for the jal instruction

---
 arch/mips/kernel/ftrace.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)
arch/mips/kernel/ftrace.c +11 -8
index 37aa767..b1b8fec 100644
@@ -23,7 +23,10 @@
 #define jump_insn_encode(op_code, addr) \
 	((unsigned int)((op_code) | (((addr) >> 2) & ADDR_MASK)))
 
-static unsigned int ftrace_nop = 0x00000000;
+#define INSN_B_1F_4 0x10000004	/* b 1f; offset = 4 */
+#define INSN_B_1F_5 0x10000005	/* b 1f; offset = 5 */
+#define INSN_NOP 0x00000000	/* nop */
+#define INSN_JAL(addr)	jump_insn_encode(JAL, addr)
 
 static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
 {
@@ -71,7 +74,7 @@ int ftrace_make_nop(struct module *mod,
 		 *  sub sp, sp, 8
 		 *                                  1: offset = 5 instructions
 		 */
-		new = 0x10000005;
+		new = INSN_B_1F_5;
 #else
 		/* lui v1, hi_16bit_of_mcount        --> b 1f (0x10000004)
 		 * addiu v1, v1, low_16bit_of_mcount
@@ -80,7 +83,7 @@ int ftrace_make_nop(struct module *mod,
 		 *  nop | move $12, ra_address | sub sp, sp, 8
 		 *                                  1: offset = 4 instructions
 		 */
-		new = 0x10000004;
+		new = INSN_B_1F_4;
 #endif
 	} else {
 		/* record/calculate it for ftrace_make_call */
@@ -88,13 +91,13 @@ int ftrace_make_nop(struct module *mod,
 			/* We can record it directly like this:
 			 *     jal_mcount = *(unsigned int *)ip;
 			 * Herein, jump over the first two nop instructions */
-			jal_mcount = jump_insn_encode(JAL, (MCOUNT_ADDR + 8));
+			jal_mcount = INSN_JAL(MCOUNT_ADDR + 8);
 		}
 
 		/* move at, ra
 		 * jalr v1		--> nop
 		 */
-		new = ftrace_nop;
+		new = INSN_NOP;
 	}
 	return ftrace_modify_code(ip, new);
 }
@@ -109,7 +112,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	/* We just need to remove the "b ftrace_stub" at the fist time! */
 	if (modified == 0) {
 		modified = 1;
-		ftrace_modify_code(addr, ftrace_nop);
+		ftrace_modify_code(addr, INSN_NOP);
 	}
 	/* ip, module: 0xc0000000, kernel: 0x80000000 */
 	new = (ip & 0x40000000) ? lui_v1 : jal_mcount;
@@ -123,7 +126,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 {
 	unsigned int new;
 
-	new = jump_insn_encode(JAL, (unsigned long)func);
+	new = INSN_JAL((unsigned long)func);
 
 	return ftrace_modify_code(FTRACE_CALL_IP, new);
 }
@@ -155,7 +158,7 @@ int ftrace_enable_ftrace_graph_caller(void)
 
 int ftrace_disable_ftrace_graph_caller(void)
 {
-	return ftrace_modify_code(FTRACE_GRAPH_CALL_IP, ftrace_nop);
+	return ftrace_modify_code(FTRACE_GRAPH_CALL_IP, INSN_NOP);
 }
 
 #endif				/* !CONFIG_DYNAMIC_FTRACE */
Signature · 1 line
1.7.0.4
#9 lore raw

PATCH v6 7/9 tracing: MIPS: Reduce the overhead of dynamic Function Tracer

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

With the help of uasm, this patch encodes the instructions of dynamic Function Tracer in ftrace_dyn_arch_init() when initializing it.

As a result, we can remove the dynamic encoding of instructions in ftrace_make_nop()/call(), ftrace_enable_ftrace_graph_caller() and remove the macro jump_insn_encode() and at last, reduces the overhead of dynamic Function Tracer and also make the source code looks better.

---
 arch/mips/kernel/ftrace.c |   93 +++++++++++++++++++++++---------------------
 1 files changed, 49 insertions(+), 44 deletions(-)
arch/mips/kernel/ftrace.c +49 -44
index b1b8fec..c4042ca 100644
@@ -2,7 +2,7 @@
  * Code for replacing ftrace calls with jumps.
  *
  * Copyright (C) 2007-2008 Steven Rostedt <[email protected]>
- * Copyright (C) 2009 DSLab, Lanzhou University, China
+ * Copyright (C) 2009, 2010 DSLab, Lanzhou University, China
  * Author: Wu Zhangjin <[email protected]>
  *
  * Thanks goes to Steven Rostedt for writing the original x86 version.
@@ -12,21 +12,46 @@
 #include <linux/init.h>
 #include <linux/ftrace.h>
 
-#include <asm/cacheflush.h>
 #include <asm/asm.h>
 #include <asm/asm-offsets.h>
+#include <asm/cacheflush.h>
+#include <asm/uasm.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 #define JAL 0x0c000000		/* jump & link: ip --> ra, jump to target */
 #define ADDR_MASK 0x03ffffff	/*  op_code|addr : 31...26|25 ....0 */
-#define jump_insn_encode(op_code, addr) \
-	((unsigned int)((op_code) | (((addr) >> 2) & ADDR_MASK)))
 
 #define INSN_B_1F_4 0x10000004	/* b 1f; offset = 4 */
 #define INSN_B_1F_5 0x10000005	/* b 1f; offset = 5 */
 #define INSN_NOP 0x00000000	/* nop */
-#define INSN_JAL(addr)	jump_insn_encode(JAL, addr)
+#define INSN_JAL(addr)	\
+	((unsigned int)(JAL | (((addr) >> 2) & ADDR_MASK)))
+
+static unsigned int insn_jal_ftrace_caller __read_mostly;
+static unsigned int insn_lui_v1_hi16_mcount __read_mostly;
+static unsigned int insn_j_ftrace_graph_caller __maybe_unused __read_mostly;
+
+static inline void ftrace_dyn_arch_init_insns(void)
+{
+	u32 *buf;
+	unsigned int v1;
+
+	/* lui v1, hi16_mcount */
+	v1 = 3;
+	buf = (u32 *)&insn_lui_v1_hi16_mcount;
+	UASM_i_LA_mostly(&buf, v1, MCOUNT_ADDR);
+
+	/* jal (ftrace_caller + 8), jump over the first two instruction */
+	buf = (u32 *)&insn_jal_ftrace_caller;
+	uasm_i_jal(&buf, (FTRACE_ADDR + 8));
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	/* j ftrace_graph_caller */
+	buf = (u32 *)&insn_j_ftrace_graph_caller;
+	uasm_i_j(&buf, (unsigned long)ftrace_graph_caller);
+#endif
+}
 
 static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
 {
@@ -43,30 +68,20 @@ static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
 	return 0;
 }
 
-static int lui_v1;
-static int jal_mcount;
-
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {
 	unsigned int new;
-	int faulted;
 	unsigned long ip = rec->ip;
 
-	/* We have compiled module with -mlong-calls, but compiled the kernel
-	 * without it, we need to cope with them respectively. */
+	/*
+	 * We have compiled module with -mlong-calls, but compiled the kernel
+	 * without it, we need to cope with them respectively.
+	 */
 	if (ip & 0x40000000) {
-		/* record it for ftrace_make_call */
-		if (lui_v1 == 0) {
-			/* lui_v1 = *(unsigned int *)ip; */
-			safe_load_code(lui_v1, ip, faulted);
-
-			if (unlikely(faulted))
-				return -EFAULT;
-		}
-
 #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
-		/* lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)
+		/*
+		 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)
 		 * addiu v1, v1, low_16bit_of_mcount
 		 * move at, ra
 		 * move $12, ra_address
@@ -76,7 +91,8 @@ int ftrace_make_nop(struct module *mod,
 		 */
 		new = INSN_B_1F_5;
 #else
-		/* lui v1, hi_16bit_of_mcount        --> b 1f (0x10000004)
+		/*
+		 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000004)
 		 * addiu v1, v1, low_16bit_of_mcount
 		 * move at, ra
 		 * jalr v1
@@ -86,36 +102,22 @@ int ftrace_make_nop(struct module *mod,
 		new = INSN_B_1F_4;
 #endif
 	} else {
-		/* record/calculate it for ftrace_make_call */
-		if (jal_mcount == 0) {
-			/* We can record it directly like this:
-			 *     jal_mcount = *(unsigned int *)ip;
-			 * Herein, jump over the first two nop instructions */
-			jal_mcount = INSN_JAL(MCOUNT_ADDR + 8);
-		}
-
-		/* move at, ra
-		 * jalr v1		--> nop
+		/*
+		 * move at, ra
+		 * jal _mcount		--> nop
 		 */
 		new = INSN_NOP;
 	}
 	return ftrace_modify_code(ip, new);
 }
 
-static int modified;	/* initialized as 0 by default */
-
 int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 {
 	unsigned int new;
 	unsigned long ip = rec->ip;
 
-	/* We just need to remove the "b ftrace_stub" at the fist time! */
-	if (modified == 0) {
-		modified = 1;
-		ftrace_modify_code(addr, INSN_NOP);
-	}
 	/* ip, module: 0xc0000000, kernel: 0x80000000 */
-	new = (ip & 0x40000000) ? lui_v1 : jal_mcount;
+	new = (ip & 0x40000000) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
 
 	return ftrace_modify_code(ip, new);
 }
@@ -133,6 +135,12 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 int __init ftrace_dyn_arch_init(void *data)
 {
+	/* Encode the instructions when booting */
+	ftrace_dyn_arch_init_insns();
+
+	/* Remove "b ftrace_stub" to ensure ftrace_caller() is executed */
+	ftrace_modify_code(MCOUNT_ADDR, INSN_NOP);
+
 	/* The return code is retured via data */
 	*(unsigned long *)data = 0;
 
@@ -145,15 +153,12 @@ int __init ftrace_dyn_arch_init(void *data)
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 extern void ftrace_graph_call(void);
-#define JMP	0x08000000	/* jump to target directly */
-#define CALL_FTRACE_GRAPH_CALLER \
-	jump_insn_encode(JMP, (unsigned long)(&ftrace_graph_caller))
 #define FTRACE_GRAPH_CALL_IP	((unsigned long)(&ftrace_graph_call))
 
 int ftrace_enable_ftrace_graph_caller(void)
 {
 	return ftrace_modify_code(FTRACE_GRAPH_CALL_IP,
-				  CALL_FTRACE_GRAPH_CALLER);
+			insn_j_ftrace_graph_caller);
 }
 
 int ftrace_disable_ftrace_graph_caller(void)
Signature · 1 line
1.7.0.4
#10 lore raw

PATCH v6 8/9 tracing: MIPS: cleanup of function graph tracer

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

This patch cleans up the comments and ftrace_get_parent_addr() of function graph tracer.

---
 arch/mips/kernel/ftrace.c |   48 ++++++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 22 deletions(-)
arch/mips/kernel/ftrace.c +26 -22
index c4042ca..628e90b 100644
@@ -146,7 +146,7 @@ int __init ftrace_dyn_arch_init(void *data)
 
 	return 0;
 }
-#endif				/* CONFIG_DYNAMIC_FTRACE */
+#endif	/* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
@@ -166,9 +166,10 @@ int ftrace_disable_ftrace_graph_caller(void)
 	return ftrace_modify_code(FTRACE_GRAPH_CALL_IP, INSN_NOP);
 }
 
-#endif				/* !CONFIG_DYNAMIC_FTRACE */
+#endif	/* CONFIG_DYNAMIC_FTRACE */
 
 #ifndef KBUILD_MCOUNT_RA_ADDRESS
+
 #define S_RA_SP	(0xafbf << 16)	/* s{d,w} ra, offset(sp) */
 #define S_R_SP	(0xafb0 << 16)  /* s{d,w} R, offset(sp) */
 #define OFFSET_MASK	0xffff	/* stack offset range: 0 ~ PT_SIZE */
@@ -182,17 +183,17 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	unsigned int code;
 	int faulted;
 
-	/* in module or kernel? */
-	if (self_addr & 0x40000000) {
-		/* module: move to the instruction "lui v1, HI_16BIT_OF_MCOUNT" */
-		ip = self_addr - 20;
-	} else {
-		/* kernel: move to the instruction "move ra, at" */
-		ip = self_addr - 12;
-	}
+	/*
+	 * For module, move the ip from calling site of mcount to the
+	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 20), but for
+	 * kernel, move to the instruction "move ra, at"(offset is 12)
+	 */
+	ip = self_addr - ((self_addr & 0x40000000) ? 20 : 12);
 
-	/* search the text until finding the non-store instruction or "s{d,w}
-	 * ra, offset(sp)" instruction */
+	/*
+	 * search the text until finding the non-store instruction or "s{d,w}
+	 * ra, offset(sp)" instruction
+	 */
 	do {
 		ip -= 4;
 
@@ -201,10 +202,11 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 
 		if (unlikely(faulted))
 			return 0;
-
-		/* If we hit the non-store instruction before finding where the
+		/*
+		 * If we hit the non-store instruction before finding where the
 		 * ra is stored, then this is a leaf function and it does not
-		 * store the ra on the stack. */
+		 * store the ra on the stack
+		 */
 		if ((code & S_R_SP) != S_R_SP)
 			return parent_addr;
 
@@ -222,7 +224,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	return 0;
 }
 
-#endif
+#endif	/* !KBUILD_MCOUNT_RA_ADDRESS */
 
 /*
  * Hook the return address and push it in the stack of return addrs
@@ -240,7 +242,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
-	/* "parent" is the stack address saved the return address of the caller
+	/*
+	 * "parent" is the stack address saved the return address of the caller
 	 * of _mcount.
 	 *
 	 * if the gcc < 4.5, a leaf function does not save the return address
@@ -262,10 +265,11 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 		goto out;
 #ifndef KBUILD_MCOUNT_RA_ADDRESS
 	parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
-							 (unsigned long)parent,
-							 fp);
-	/* If fails when getting the stack address of the non-leaf function's
-	 * ra, stop function graph tracer and return */
+			(unsigned long)parent, fp);
+	/*
+	 * If fails when getting the stack address of the non-leaf function's
+	 * ra, stop function graph tracer and return
+	 */
 	if (parent == 0)
 		goto out;
 #endif
@@ -292,4 +296,4 @@ out:
 	ftrace_graph_stop();
 	WARN_ON(1);
 }
-#endif				/* CONFIG_FUNCTION_GRAPH_TRACER */
+#endif	/* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
1.7.0.4
#11 lore raw

PATCH v6 9/9 tracing: MIPS: cleanup of the address space checking

In reply to Wu Zhangjin

From: Wu Zhangjin <[email protected]>

This patch adds an inline function in_module() to check which space the instruction pointer in, kernel space or module space.

Note: This may not work when the kernel is compiled with -msym32.

---
 arch/mips/kernel/ftrace.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)
arch/mips/kernel/ftrace.c +14 -3
index 628e90b..37f15b6 100644
@@ -17,6 +17,17 @@
 #include <asm/cacheflush.h>
 #include <asm/uasm.h>
 
+/*
+ * If the Instruction Pointer is in module space (0xc0000000), return true;
+ * otherwise, it is in kernel space (0x80000000), return false.
+ *
+ * FIXME: This may not work when the kernel is compiled with -msym32.
+ */
+static inline int in_module(unsigned long ip)
+{
+	return ip & 0x40000000;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 #define JAL 0x0c000000		/* jump & link: ip --> ra, jump to target */
@@ -78,7 +89,7 @@ int ftrace_make_nop(struct module *mod,
 	 * We have compiled module with -mlong-calls, but compiled the kernel
 	 * without it, we need to cope with them respectively.
 	 */
-	if (ip & 0x40000000) {
+	if (in_module(ip)) {
 #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
 		/*
 		 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)
@@ -117,7 +128,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned long ip = rec->ip;
 
 	/* ip, module: 0xc0000000, kernel: 0x80000000 */
-	new = (ip & 0x40000000) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
+	new = in_module(ip) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
 
 	return ftrace_modify_code(ip, new);
 }
@@ -188,7 +199,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 20), but for
 	 * kernel, move to the instruction "move ra, at"(offset is 12)
 	 */
-	ip = self_addr - ((self_addr & 0x40000000) ? 20 : 12);
+	ip = self_addr - (in_module(self_addr) ? 20 : 12);
 
 	/*
 	 * search the text until finding the non-store instruction or "s{d,w}
Signature · 1 line
1.7.0.4
#12 lore raw

Re: [PATCH 9/9] tracing: MIPS: cleanup of the address space checking

Quoted 7 lines · Wu Zhangjin

From: Wu Zhangjin<[email protected]>

This patch adds an inline function in_module() to check which space the instruction pointer in, kernel space or module space.

Note: This may not work when the kernel is compiled with -msym32.

The kernel is always compiled with -msym32, so the patch is a bit pointless.

Quoted 14 lines
---
  arch/mips/kernel/ftrace.c |   17 ++++++++++++++---
  1 files changed, 14 insertions(+), 3 deletions(-)
arch/mips/kernel/ftrace.c +1 -0
index 628e90b..37f15b6 100644
@@ -17,6 +17,17 @@
  #include<asm/cacheflush.h>
  #include<asm/uasm.h>
 
+/*

arch/mips/kernel/ftrace.c line 30

Quoted 10 lines
arch/mips/kernel/ftrace.c +10 -0
+ * If the Instruction Pointer is in module space (0xc0000000), return true;
+ * otherwise, it is in kernel space (0x80000000), return false.
+ *
+ * FIXME: This may not work when the kernel is compiled with -msym32.
+ */
+static inline int in_module(unsigned long ip)
+{
+	return ip&  0x40000000;
+}
+

How about (untested):

static inline int in_module(unsigned long ip)

{
	return ip < _text || ip > _etext;
}

But why do we even care? Can't we just probe the function prologue and determine from that what needs to be done?

David Daney

Quoted 30 lines
arch/mips/kernel/ftrace.c +3 -3
  #ifdef CONFIG_DYNAMIC_FTRACE
 
  #define JAL 0x0c000000		/* jump&  link: ip -->  ra, jump to target */
@@ -78,7 +89,7 @@ int ftrace_make_nop(struct module *mod,
  	 * We have compiled module with -mlong-calls, but compiled the kernel
  	 * without it, we need to cope with them respectively.
  	 */
-	if (ip&  0x40000000) {
+	if (in_module(ip)) {
  #if defined(KBUILD_MCOUNT_RA_ADDRESS)&&  defined(CONFIG_32BIT)
  		/*
  		 * lui v1, hi_16bit_of_mcount        -->  b 1f (0x10000005)
@@ -117,7 +128,7 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
  	unsigned long ip = rec->ip;
 
  	/* ip, module: 0xc0000000, kernel: 0x80000000 */
-	new = (ip&  0x40000000) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
+	new = in_module(ip) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
 
  	return ftrace_modify_code(ip, new);
  }
@@ -188,7 +199,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
  	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 20), but for
  	 * kernel, move to the instruction "move ra, at"(offset is 12)
  	 */
-	ip = self_addr - ((self_addr&  0x40000000) ? 20 : 12);
+	ip = self_addr - (in_module(self_addr) ? 20 : 12);
 
  	/*
  	 * search the text until finding the non-store instruction or "s{d,w}
#13 lore raw

Re: [PATCH 9/9] tracing: MIPS: cleanup of the address space checking

Quoted 12 lines · David Daney
Quoted 7 lines · Wu Zhangjin

From: Wu Zhangjin<[email protected]>

This patch adds an inline function in_module() to check which space the instruction pointer in, kernel space or module space.

Note: This may not work when the kernel is compiled with -msym32.

The kernel is always compiled with -msym32, so the patch is a bit pointless.

In reality, with -msym32, it works well on my machine, but seems you and some other people told me that it may not work when the kernel and module space are the same with -msym32 and some other options. perhaps I need to change the comments to just:

Note: This will not work when the kernel and module space are the same.

If the kernel and module space are the same, We just need to modify the recording of the calling site to _mcount in scripts/recordmcount.pl and tune the related address handling in ftrace_make_nop()/ftrace_make_call().

But I have no such situation to test, how can I simply let the module has the same address space as the kernel. without -mlong-calls? and should we change arch/mips/kernel/vmlinux.lds.S and arch/mips/kernel/module.c?

If the module space and kernel space are the same, we may remove the long call from the module space to kernel space to speedup the function call.

Quoted 24 lines
Quoted 10 lines
arch/mips/kernel/ftrace.c +1 -0
index 628e90b..37f15b6 100644
@@ -17,6 +17,17 @@
  #include<asm/cacheflush.h>
  #include<asm/uasm.h>
 
+/*

arch/mips/kernel/ftrace.c line 30

Quoted 10 lines
arch/mips/kernel/ftrace.c +10 -0
+ * If the Instruction Pointer is in module space (0xc0000000), return true;
+ * otherwise, it is in kernel space (0x80000000), return false.
+ *
+ * FIXME: This may not work when the kernel is compiled with -msym32.
+ */
+static inline int in_module(unsigned long ip)
+{
+	return ip&  0x40000000;
+}
+

How about (untested):

Quoted 5 lines

static inline int in_module(unsigned long ip)

{
	return ip < _text || ip > _etext;
}

It may work, thanks!

Quoted 3 lines

But why do we even care? Can't we just probe the function prologue and determine from that what needs to be done?

Yes, it should work via checking the instructions in the memory before the ip but I think a lighter solution is better.

Thanks & Regards,
Wu Zhangjin

#14 lore raw

Re: [PATCH 9/9] tracing: MIPS: cleanup of the address space checking

In reply to David Daney
Quoted 1 line · David Daney

The kernel is always compiled with -msym32, so the patch is a bit pointless.

Not quite true. Some systems only have enough memory for the exception vectors in the low 512MB of physical address space, so these can't use an -msym32 kernel.

My general impression is that hardware designers "design" address maps by throwing darts over their shoulder after a few pints ;-)

Ralf

#15 lore raw

Re: [PATCH 9/9] tracing: MIPS: cleanup of the address space checking

Quoted 11 lines · Ralf Baechle
Quoted 1 line · David Daney

The kernel is always compiled with -msym32, so the patch is a bit pointless.

Not quite true. Some systems only have enough memory for the exception vectors in the low 512MB of physical address space, so these can't use an -msym32 kernel.

My general impression is that hardware designers "design" address maps by throwing darts over their shoulder after a few pints ;-)

Well it was mostly a rhetorical point. Of course -msym32 is not universal, but it is more common than not I would say. Because of that, we should take a few more minutes and figure out how to make ftrace work well with it.

David Daney