Misc updates for Ftrace of MIPS

PATCH Applied Wu Zhangjin 4 participants 18 messages Jan 19, 2011 – Jan 26, 2011

Subsystems (2)

MIPS ftrace

lore

Share & tools

lore

Wu Zhangjin

author

2011-01-19 19:28 UTC #0 lore raw

cover Misc updates for Ftrace of MIPS

Hi, Ralf and Steve

This adds several patches for Ftrace of MIPS, most of them are trivial cleanups, the last one fixes the useful set_graph_function interface provided by function graph tracer.

The 1st two patches has been sent before, here just resend them.

Thanks very much to Zhiping for reporting the set_graph_function problem.

Regards,
Wu Zhangjin

Wu Zhangjin (5):
tracing, MIPS: Speed up function graph tracer tracing, MIPS: Substitute in_kernel_space() for in_module() tracing, MIPS: Clean up prepare_ftrace_return() tracing, MIPS: Clean up ftrace_make_nop()
tracing, MIPS: Fix set_graph_function of function graph tracer

 arch/mips/kernel/ftrace.c |  179 ++++++++++++++++++++++++---------------------
 1 files changed, 95 insertions(+), 84 deletions(-)

Wu Zhangjin

author

2011-01-19 19:29 UTC #1 lore raw

PATCH 1/5 tracing, MIPS: reduce one instruction for function graph tracer

This simply moves the "ip-=4" statement down to the end of the do { ... } while (...); loop, which reduces one unneeded subtration and the subsequent memory loading and comparation. as a result, speed up the function a little.

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

arch/mips/kernel/ftrace.c +7 -7

index 5a84a1f..635c1dc 100644
@@ -200,19 +200,17 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	int faulted;
 
 	/*
-	 * 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)
+	 * For module, move the ip from calling site of mcount after the
+	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
+	 * kernel, move after the instruction "move ra, at"(offset is 16)
 	 */
-	ip = self_addr - (in_module(self_addr) ? 20 : 12);
+	ip = self_addr - (in_module(self_addr) ? 24 : 16);
 
 	/*
 	 * search the text until finding the non-store instruction or "s{d,w}
 	 * ra, offset(sp)" instruction
 	 */
 	do {
-		ip -= 4;
-
 		/* get the code at "ip": code = *(unsigned int *)ip; */
 		safe_load_code(code, ip, faulted);

@@ -226,7 +224,9 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 		if ((code & S_R_SP) != S_R_SP)
 			return parent_addr;
 
-	} while (((code & S_RA_SP) != S_RA_SP));
+		/* Move to the next instruction */
+		ip -= 4;
+	} while ((code & S_RA_SP) != S_RA_SP);
 
 	sp = fp + (code & OFFSET_MASK);

Signature · 1 line

1.7.1

wu zhangjin

author

2011-01-19 19:33 UTC #2 lore raw

Re: [PATCH 1/5] tracing, MIPS: reduce one instruction for function graph tracer

Sorry, please ignore this one.

Quoted 53 lines · Wu Zhangjin

Signed-off-by Wu Zhangjin <[email protected]>

---
arch/mips/kernel/ftrace.c | 14 +++++++-------

 1 files changed, 7 insertions(+), 7 deletions(-)

arch/mips/kernel/ftrace.c +7 -8

index 5a84a1f..635c1dc 100644
@@ -200,19 +200,17 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
        int faulted;
 
        /*
-        * 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)
+        * For module, move the ip from calling site of mcount after the
+        * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
+        * kernel, move after the instruction "move ra, at"(offset is 16)
         */
-       ip = self_addr - (in_module(self_addr) ? 20 : 12);
+       ip = self_addr - (in_module(self_addr) ? 24 : 16);
 
        /*
         * search the text until finding the non-store instruction or "s{d,w}
         * ra, offset(sp)" instruction
         */
        do {
-               ip -= 4;
-
                /* get the code at "ip": code = *(unsigned int *)ip; */
                safe_load_code(code, ip, faulted);

@@ -226,7 +224,9 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
                if ((code & S_R_SP) != S_R_SP)
                        return parent_addr;
 
-       } while (((code & S_RA_SP) != S_RA_SP));
+               /* Move to the next instruction */
+               ip -= 4;
+       } while ((code & S_RA_SP) != S_RA_SP);
 
        sp = fp + (code & OFFSET_MASK);
 
--

1.7.1

Wu Zhangjin

author

2011-01-19 19:30 UTC #3 lore raw

PATCH 1/5 tracing, MIPS: Speed up function graph tracer

In reply to Wu Zhangjin

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

arch/mips/kernel/ftrace.c +7 -7

index 5a84a1f..635c1dc 100644
@@ -200,19 +200,17 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	int faulted;
 
 	/*
-	 * 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)
+	 * For module, move the ip from calling site of mcount after the
+	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
+	 * kernel, move after the instruction "move ra, at"(offset is 16)
 	 */
-	ip = self_addr - (in_module(self_addr) ? 20 : 12);
+	ip = self_addr - (in_module(self_addr) ? 24 : 16);
 
 	/*
 	 * search the text until finding the non-store instruction or "s{d,w}
 	 * ra, offset(sp)" instruction
 	 */
 	do {
-		ip -= 4;
-
 		/* get the code at "ip": code = *(unsigned int *)ip; */
 		safe_load_code(code, ip, faulted);

@@ -226,7 +224,9 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 		if ((code & S_R_SP) != S_R_SP)
 			return parent_addr;
 
-	} while (((code & S_RA_SP) != S_RA_SP));
+		/* Move to the next instruction */
+		ip -= 4;
+	} while ((code & S_RA_SP) != S_RA_SP);
 
 	sp = fp + (code & OFFSET_MASK);

Signature · 1 line

1.7.1

Ralf Baechle

Applied

2011-01-24 13:51 UTC #4 lore raw

Re: [PATCH 1/5] tracing, MIPS: Speed up function graph tracer

Applied,

Ralf

Wu Zhangjin

author

2011-01-19 19:29 UTC #5 lore raw

PATCH 2/5 tracing, MIPS: replace in_module() with a generic in_kernel_space()

In reply to Wu Zhangjin

The old in_module() may not work in some situations(e.g. when module & kernel are in the same address space when CONFIG_MAPPED_KERNEL=y), The in_kernel_space() is more generic and it is also easy to be implemented via cloning the existing core_kernel_text(), so, replace the in_module() with in_kernel_space().

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   54 ++++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

arch/mips/kernel/ftrace.c +27 ~~-27~~

index 635c1dc..5970286 100644
@@ -17,21 +17,7 @@
 #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 will not work when the kernel space and module space are the
- * same. If they are the same, we need to modify scripts/recordmcount.pl,
- * ftrace_make_nop/call() and the other related parts to ensure the
- * enabling/disabling of the calling site to _mcount is right for both kernel
- * and module.
- */
-
-static inline int in_module(unsigned long ip)
-{
-	return ip & 0x40000000;
-}
+#include <asm-generic/sections.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE

@@ -69,6 +55,20 @@ static inline void ftrace_dyn_arch_init_insns(void)
 #endif
 }
 
+/*
+ * Check if the address is in kernel space
+ *
+ * Clone core_kernel_text() from kernel/extable.c, but doesn't call
+ * init_kernel_text() for Ftrace doesn't trace functions in init sections.
+ */
+static inline int in_kernel_space(unsigned long ip)
+{
+	if (ip >= (unsigned long)_stext &&
+	    ip <= (unsigned long)_etext)
+		return 1;
+	return 0;
+}
+
 static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
 {
 	int faulted;

@@ -91,10 +91,16 @@ int ftrace_make_nop(struct module *mod,
 	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.
+	 * If ip is in kernel space, no long call, otherwise, long call is
+	 * needed.
 	 */
-	if (in_module(ip)) {
+	if (in_kernel_space(ip)) {
+		/*
+		 * move at, ra
+		 * jal _mcount		--> nop
+		 */
+		new = INSN_NOP;
+	} else {
 #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
 		/*
 		 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)

@@ -117,12 +123,6 @@ int ftrace_make_nop(struct module *mod,
 		 */
 		new = INSN_B_1F_4;
 #endif
-	} else {
-		/*
-		 * move at, ra
-		 * jal _mcount		--> nop
-		 */
-		new = INSN_NOP;
 	}
 	return ftrace_modify_code(ip, new);
 }

@@ -132,8 +132,8 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned int new;
 	unsigned long ip = rec->ip;
 
-	/* ip, module: 0xc0000000, kernel: 0x80000000 */
-	new = in_module(ip) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
+	new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
+		insn_lui_v1_hi16_mcount;
 
 	return ftrace_modify_code(ip, new);
 }

@@ -204,7 +204,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
 	 * kernel, move after the instruction "move ra, at"(offset is 16)
 	 */
-	ip = self_addr - (in_module(self_addr) ? 24 : 16);
+	ip = self_addr - (in_kernel_space(self_addr) ? 16 : 24);
 
 	/*
 	 * search the text until finding the non-store instruction or "s{d,w}

Signature · 1 line

1.7.1

wu zhangjin

author

2011-01-19 19:32 UTC #6 lore raw

Re: [PATCH 2/5] tracing, MIPS: replace in_module() with a generic in_kernel_space()

And ignore this one too.

Quoted 116 lines · Wu Zhangjin

Signed-off-by Wu Zhangjin <[email protected]>

---
arch/mips/kernel/ftrace.c | 54 ++++++++++++++++++++++----------------------

 1 files changed, 27 insertions(+), 27 deletions(-)

arch/mips/kernel/ftrace.c +27 ~~-28~~

index 635c1dc..5970286 100644
@@ -17,21 +17,7 @@
  #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 will not work when the kernel space and module space are the
- * same. If they are the same, we need to modify scripts/recordmcount.pl,
- * ftrace_make_nop/call() and the other related parts to ensure the
- * enabling/disabling of the calling site to _mcount is right for both kernel
- * and module.
- */
-
-static inline int in_module(unsigned long ip)
-{
-       return ip & 0x40000000;
-}
+#include <asm-generic/sections.h>
 
  #ifdef CONFIG_DYNAMIC_FTRACE

@@ -69,6 +55,20 @@ static inline void ftrace_dyn_arch_init_insns(void)
  #endif
  }
 
+/*
+ * Check if the address is in kernel space
+ *
+ * Clone core_kernel_text() from kernel/extable.c, but doesn't call
+ * init_kernel_text() for Ftrace doesn't trace functions in init sections.
+ */
+static inline int in_kernel_space(unsigned long ip)
+{
+       if (ip >= (unsigned long)_stext &&
+           ip <= (unsigned long)_etext)
+               return 1;
+       return 0;
+}
+
  static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
  {
        int faulted;

@@ -91,10 +91,16 @@ int ftrace_make_nop(struct module *mod,
        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.
+        * If ip is in kernel space, no long call, otherwise, long call is
+        * needed.
         */
-       if (in_module(ip)) {
+       if (in_kernel_space(ip)) {
+               /*
+                * move at, ra
+                * jal _mcount          --> nop
+                */
+               new = INSN_NOP;
+       } else {
  #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
                /*
                 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)

@@ -117,12 +123,6 @@ int ftrace_make_nop(struct module *mod,
                 */
                new = INSN_B_1F_4;
  #endif
-       } else {
-               /*
-                * move at, ra
-                * jal _mcount          --> nop
-                */
-               new = INSN_NOP;
        }
        return ftrace_modify_code(ip, new);
  }

@@ -132,8 +132,8 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
        unsigned int new;
        unsigned long ip = rec->ip;
 
-       /* ip, module: 0xc0000000, kernel: 0x80000000 */
-       new = in_module(ip) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
+       new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
+               insn_lui_v1_hi16_mcount;
 
        return ftrace_modify_code(ip, new);
  }

@@ -204,7 +204,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
         * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
         * kernel, move after the instruction "move ra, at"(offset is 16)
         */
-       ip = self_addr - (in_module(self_addr) ? 24 : 16);
+       ip = self_addr - (in_kernel_space(self_addr) ? 16 : 24);
 
        /*
         * search the text until finding the non-store instruction or "s{d,w}
--

1.7.1

Wu Zhangjin

author

2011-01-19 19:30 UTC #7 lore raw

PATCH 2/5 tracing, MIPS: Substitute in_kernel_space() for in_module()

In reply to Wu Zhangjin

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   54 ++++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

arch/mips/kernel/ftrace.c +27 ~~-27~~

index 635c1dc..5970286 100644
@@ -17,21 +17,7 @@
 #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 will not work when the kernel space and module space are the
- * same. If they are the same, we need to modify scripts/recordmcount.pl,
- * ftrace_make_nop/call() and the other related parts to ensure the
- * enabling/disabling of the calling site to _mcount is right for both kernel
- * and module.
- */
-
-static inline int in_module(unsigned long ip)
-{
-	return ip & 0x40000000;
-}
+#include <asm-generic/sections.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE

@@ -69,6 +55,20 @@ static inline void ftrace_dyn_arch_init_insns(void)
 #endif
 }
 
+/*
+ * Check if the address is in kernel space
+ *
+ * Clone core_kernel_text() from kernel/extable.c, but doesn't call
+ * init_kernel_text() for Ftrace doesn't trace functions in init sections.
+ */
+static inline int in_kernel_space(unsigned long ip)
+{
+	if (ip >= (unsigned long)_stext &&
+	    ip <= (unsigned long)_etext)
+		return 1;
+	return 0;
+}
+
 static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
 {
 	int faulted;

@@ -91,10 +91,16 @@ int ftrace_make_nop(struct module *mod,
 	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.
+	 * If ip is in kernel space, no long call, otherwise, long call is
+	 * needed.
 	 */
-	if (in_module(ip)) {
+	if (in_kernel_space(ip)) {
+		/*
+		 * move at, ra
+		 * jal _mcount		--> nop
+		 */
+		new = INSN_NOP;
+	} else {
 #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
 		/*
 		 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000005)

@@ -117,12 +123,6 @@ int ftrace_make_nop(struct module *mod,
 		 */
 		new = INSN_B_1F_4;
 #endif
-	} else {
-		/*
-		 * move at, ra
-		 * jal _mcount		--> nop
-		 */
-		new = INSN_NOP;
 	}
 	return ftrace_modify_code(ip, new);
 }

@@ -132,8 +132,8 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned int new;
 	unsigned long ip = rec->ip;
 
-	/* ip, module: 0xc0000000, kernel: 0x80000000 */
-	new = in_module(ip) ? insn_lui_v1_hi16_mcount : insn_jal_ftrace_caller;
+	new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
+		insn_lui_v1_hi16_mcount;
 
 	return ftrace_modify_code(ip, new);
 }

@@ -204,7 +204,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
 	 * kernel, move after the instruction "move ra, at"(offset is 16)
 	 */
-	ip = self_addr - (in_module(self_addr) ? 24 : 16);
+	ip = self_addr - (in_kernel_space(self_addr) ? 16 : 24);
 
 	/*
 	 * search the text until finding the non-store instruction or "s{d,w}

Signature · 1 line

1.7.1

Ralf Baechle

2011-01-24 13:57 UTC #8 lore raw

Re: [PATCH 2/5] tracing, MIPS: Substitute in_kernel_space() for in_module()

arch/mips/kernel/ftrace.c int ftrace_make_nop(struct module *mod, · line 96

Quoted 9 lines · Wu Zhangjin

arch/mips/kernel/ftrace.c +2 -2

@@ -91,10 +91,16 @@ int ftrace_make_nop(struct module *mod,
 	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.
+	 * If ip is in kernel space, no long call, otherwise, long call is
+	 * needed.
 	 */

Or even better, just check if the destination is in the same 28-bit segment of address space. Something like:

	if ((src ^ dst) >> 28)  {
		/* out of range of simple R_MIPS_26 relocation */
	}

That way you no longer rely on a particular address layout - and there are plans to change the address space layout eventually!

Ralf

wu zhangjin

author

2011-01-26 12:36 UTC #9 lore raw

Re: [PATCH 2/5] tracing, MIPS: Substitute in_kernel_space() for in_module()

Quoted 15 lines · Ralf Baechle

arch/mips/kernel/ftrace.c int ftrace_make_nop(struct module *mod, · line 96
Quoted 9 lines · Wu Zhangjin
arch/mips/kernel/ftrace.c +2 -2
@@ -91,10 +91,16 @@ int ftrace_make_nop(struct module *mod,
       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.
+      * If ip is in kernel space, no long call, otherwise, long call is
+      * needed.
        */
Or even better, just check if the destination is in the same 28-bit segment of address space. Something like:

Quoted 3 lines

if ((src ^ dst) >> 28) {
/* out of range of simple R_MIPS_26 relocation */ }

Yeah, will check if the following statement works:

if ((rec->ip ^ _mcount) >> 28) {
    ...
}

This check may be faster and clearer ;-)

Quoted 3 lines

That way you no longer rely on a particular address layout - and there are plans to change the address space layout eventually!

Yeah, we really need an address space independent method.

Thanks & Regards,
Wu Zhangjin

Quoted 3 lines

Ralf

Wu Zhangjin

author

2011-01-19 19:31 UTC #10 lore raw

PATCH 3/5 tracing, MIPS: Clean up prepare_ftrace_return()

In reply to Wu Zhangjin

The old prepare_ftrace_return() for MIPS is confused and have introduced some problem. This patch cleans up the names of the arguments, variables and related functions.

For MIPS, the 2nd argument of prepare_ftrace_return() is not really the 'selfpc' described in ftrace-design.txt but instead it is the self return address. This did break the compatibility of the generic interface but really reduced one unneeded calculation for to get the current function name, the parent return address and the self return address are enough, no need to tranform the self return address to the self address.

But set_graph_function of function graph tracer is an exception, it does need the 2nd argument of prepare_ftrace_return() as 'selfpc', for it will use 'selfpc' to match user's configuration of function graph entries, but in reality, it doesn't need the 'selfpc' but the recorded ip address of the mcount calling site in the __mcount_loc section. So, the 2nd argument of prepare_ftrace_return() is not important, the real requirement is the right recorded ip address should be calculated and assign to trace.func, this will be fixed in the next patches.

Reported-by Zhiping Zhong <[email protected]>

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   52 +++++++++++++++++++++-----------------------
 1 files changed, 25 insertions(+), 27 deletions(-)

arch/mips/kernel/ftrace.c +25 ~~-27~~

index 5970286..40ef34c 100644
@@ -190,21 +190,19 @@ int ftrace_disable_ftrace_graph_caller(void)
 #define S_R_SP	(0xafb0 << 16)  /* s{d,w} R, offset(sp) */
 #define OFFSET_MASK	0xffff	/* stack offset range: 0 ~ PT_SIZE */
 
-unsigned long ftrace_get_parent_addr(unsigned long self_addr,
-				     unsigned long parent,
-				     unsigned long parent_addr,
-				     unsigned long fp)
+unsigned long ftrace_get_parent_ra_addr(unsigned long self_ra, unsigned long
+		old_parent_ra, unsigned long parent_ra_addr, unsigned long fp)
 {
-	unsigned long sp, ip, ra;
+	unsigned long sp, ip, tmp;
 	unsigned int code;
 	int faulted;
 
 	/*
-	 * For module, move the ip from calling site of mcount after the
+	 * For module, move the ip from the return address after the
 	 * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
 	 * kernel, move after the instruction "move ra, at"(offset is 16)
 	 */
-	ip = self_addr - (in_kernel_space(self_addr) ? 16 : 24);
+	ip = self_ra - (in_kernel_space(self_ra) ? 16 : 24);
 
 	/*
 	 * search the text until finding the non-store instruction or "s{d,w}

@@ -222,7 +220,7 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 		 * store the ra on the stack
 		 */
 		if ((code & S_R_SP) != S_R_SP)
-			return parent_addr;
+			return parent_ra_addr;
 
 		/* Move to the next instruction */
 		ip -= 4;

@@ -230,12 +228,12 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 
 	sp = fp + (code & OFFSET_MASK);
 
-	/* ra = *(unsigned long *)sp; */
-	safe_load_stack(ra, sp, faulted);
+	/* tmp = *(unsigned long *)sp; */
+	safe_load_stack(tmp, sp, faulted);
 	if (unlikely(faulted))
 		return 0;
 
-	if (ra == parent)
+	if (tmp == old_parent_ra)
 		return sp;
 	return 0;
 }

@@ -246,10 +244,10 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
  * Hook the return address and push it in the stack of return addrs
  * in current thread info.
  */
-void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
+void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 			   unsigned long fp)
 {
-	unsigned long old;
+	unsigned long old_parent_ra;
 	struct ftrace_graph_ent trace;
 	unsigned long return_hooker = (unsigned long)
 	    &return_to_handler;

@@ -259,8 +257,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 		return;
 
 	/*
-	 * "parent" is the stack address saved the return address of the caller
-	 * of _mcount.
+	 * "parent_ra_addr" 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
 	 * in the stack address, so, we "emulate" one in _mcount's stack space,

@@ -275,37 +273,37 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 * do it in ftrace_graph_caller of mcount.S.
 	 */
 
-	/* old = *parent; */
-	safe_load_stack(old, parent, faulted);
+	/* old_parent_ra = *parent_ra_addr; */
+	safe_load_stack(old_parent_ra, parent_ra_addr, faulted);
 	if (unlikely(faulted))
 		goto out;
 #ifndef KBUILD_MCOUNT_RA_ADDRESS
-	parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
-			(unsigned long)parent, fp);
+	parent_ra_addr = (unsigned long *)ftrace_get_parent_ra_addr(self_ra,
+			old_parent_ra, (unsigned long)parent_ra_addr, fp);
 	/*
 	 * If fails when getting the stack address of the non-leaf function's
 	 * ra, stop function graph tracer and return
 	 */
-	if (parent == 0)
+	if (parent_ra_addr == 0)
 		goto out;
 #endif
-	/* *parent = return_hooker; */
-	safe_store_stack(return_hooker, parent, faulted);
+	/* *parent_ra_addr = return_hooker; */
+	safe_store_stack(return_hooker, parent_ra_addr, faulted);
 	if (unlikely(faulted))
 		goto out;
 
-	if (ftrace_push_return_trace(old, self_addr, &trace.depth, fp) ==
-	    -EBUSY) {
-		*parent = old;
+	if (ftrace_push_return_trace(old_parent_ra, self_ra, &trace.depth, fp)
+	    == -EBUSY) {
+		*parent_ra_addr = old_parent_ra;
 		return;
 	}
 
-	trace.func = self_addr;
+	trace.func = self_ra;
 
 	/* Only trace if the calling function expects to */
 	if (!ftrace_graph_entry(&trace)) {
 		current->curr_ret_stack--;
-		*parent = old;
+		*parent_ra_addr = old_parent_ra;
 	}
 	return;
 out:

Signature · 1 line

1.7.1

Ralf Baechle

Applied

2011-01-24 14:22 UTC #11 lore raw

Re: [PATCH 3/5] tracing, MIPS: Clean up prepare_ftrace_return()

Applied. Thanks,

Ralf

Wu Zhangjin

author

2011-01-19 19:31 UTC #12 lore raw

PATCH 4/5 tracing, MIPS: Clean up ftrace_make_nop()

In reply to Wu Zhangjin

This moves the comments out of ftrace_make_nop() and make it cleaner. At the same time, a macro MCOUNT_OFFSET_INSNS is defined for sharing with the next patch.

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   70 ++++++++++++++++++++++++--------------------
 1 files changed, 38 insertions(+), 32 deletions(-)

arch/mips/kernel/ftrace.c +38 ~~-32~~

index 40ef34c..bc91e4a 100644
@@ -24,8 +24,6 @@
 #define JAL 0x0c000000		/* jump & link: ip --> ra, jump to target */
 #define ADDR_MASK 0x03ffffff	/*  op_code|addr : 31...26|25 ....0 */
 
-#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)	\
 	((unsigned int)(JAL | (((addr) >> 2) & ADDR_MASK)))

@@ -84,6 +82,42 @@ static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
 	return 0;
 }
 
+/*
+ * The details about the calling site of mcount on MIPS
+ *
+ * 1. For kernel:
+ *
+ * move at, ra
+ * jal _mcount		--> nop
+ *
+ * 2. For modules:
+ *
+ * 2.1 For KBUILD_MCOUNT_RA_ADDRESS and 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
+ * 2.2 For the Other situations
+ *
+ * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000004)
+ * addiu v1, v1, low_16bit_of_mcount
+ * move at, ra
+ * jalr v1
+ *  nop | move $12, ra_address | sub sp, sp, 8
+ *                                  1: offset = 4 instructions
+ */
+
+#if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
+#define MCOUNT_OFFSET_INSNS 5
+#else
+#define MCOUNT_OFFSET_INSNS 4
+#endif
+#define INSN_B_1F (0x10000000 | MCOUNT_OFFSET_INSNS)
+
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {

@@ -94,36 +128,8 @@ int ftrace_make_nop(struct module *mod,
 	 * If ip is in kernel space, no long call, otherwise, long call is
 	 * needed.
 	 */
-	if (in_kernel_space(ip)) {
-		/*
-		 * move at, ra
-		 * jal _mcount		--> nop
-		 */
-		new = INSN_NOP;
-	} else {
-#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 = INSN_B_1F_5;
-#else
-		/*
-		 * lui v1, hi_16bit_of_mcount        --> b 1f (0x10000004)
-		 * addiu v1, v1, low_16bit_of_mcount
-		 * move at, ra
-		 * jalr v1
-		 *  nop | move $12, ra_address | sub sp, sp, 8
-		 *                                  1: offset = 4 instructions
-		 */
-		new = INSN_B_1F_4;
-#endif
-	}
+	new = in_kernel_space(ip) ? INSN_NOP : INSN_B_1F;
+
 	return ftrace_modify_code(ip, new);
 }

Signature · 1 line

1.7.1

Wu Zhangjin

author

2011-01-19 19:32 UTC #13 lore raw

PATCH 5/5 tracing, MIPS: Fix set_graph_function of function graph tracer

In reply to Wu Zhangjin

trace.func should be set to the recorded ip of the mcount calling site in the __mcount_loc section to filter the function entries configured through the tracing/set_graph_function interface, but before, this is set to the self_ra(the return address of mcount), which has made set_graph_function not work as expects.

This fixes it via calculating the right recorded ip in the __mcount_loc section and assign it to trace.func.

Reported-by Zhiping Zhong <[email protected]>

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/kernel/ftrace.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

arch/mips/kernel/ftrace.c +9 -2

index bc91e4a..62775d7 100644
@@ -257,7 +257,7 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 	struct ftrace_graph_ent trace;
 	unsigned long return_hooker = (unsigned long)
 	    &return_to_handler;
-	int faulted;
+	int faulted, insns;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;

@@ -304,7 +304,14 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 		return;
 	}
 
-	trace.func = self_ra;
+	/*
+	 * Get the recorded ip of the current mcount calling site in the
+	 * __mcount_loc section, which will be used to filter the function
+	 * entries configured through the tracing/set_graph_function interface.
+	 */
+
+	insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
+	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
 
 	/* Only trace if the calling function expects to */
 	if (!ftrace_graph_entry(&trace)) {

Signature · 1 line

1.7.1

Sergei Shtylyov

2011-01-20 11:04 UTC #14 lore raw

Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer

Hello.

Quoted 5 lines · Wu Zhangjin

trace.func should be set to the recorded ip of the mcount calling site in the __mcount_loc section to filter the function entries configured through the tracing/set_graph_function interface, but before, this is set to the self_ra(the return address of mcount), which has made set_graph_function not work as expects.

Expected?

Quoted 13 lines

This fixes it via calculating the right recorded ip in the __mcount_loc section and assign it to trace.func.
Reported-by Zhiping Zhong<[email protected]>
Signed-off-by Wu Zhangjin<[email protected]>
---
  arch/mips/kernel/ftrace.c |   11 +++++++++--
  1 files changed, 9 insertions(+), 2 deletions(-)
arch/mips/kernel/ftrace.c +0 -0
index bc91e4a..62775d7 100644

[...]

arch/mips/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra, · line 313

Quoted 12 lines

arch/mips/kernel/ftrace.c +7 -1

@@ -304,7 +304,14 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
  		return;
  	}
 
-	trace.func = self_ra;
+	/*
+	 * Get the recorded ip of the current mcount calling site in the
+	 * __mcount_loc section, which will be used to filter the function
+	 * entries configured through the tracing/set_graph_function interface.
+	 */
+
+	insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);

Unneeded parens.

arch/mips/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra, · line 314

Quoted 1 line

arch/mips/kernel/ftrace.c +1 -0
+	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);

Here too.

WBR, Sergei

wu zhangjin

author

2011-01-20 12:46 UTC #15 lore raw

Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer

Quoted 11 lines · Sergei Shtylyov

Hello.
Quoted 5 lines · Wu Zhangjin
trace.func should be set to the recorded ip of the mcount calling site in the __mcount_loc section to filter the function entries configured through the tracing/set_graph_function interface, but before, this is set to the self_ra(the return address of mcount), which has made set_graph_function not work as expects.
Expected?

Yeah ;-)

Quoted 22 lines

Quoted 14 lines
This fixes it via calculating the right recorded ip in the __mcount_loc section and assign it to trace.func.
Reported-by Zhiping Zhong<[email protected]>
Signed-off-by Wu Zhangjin<[email protected]>
---
arch/mips/kernel/ftrace.c | 11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)
arch/mips/kernel/ftrace.c +0 -0
index bc91e4a..62775d7 100644
[...]
Quoted 6 lines
arch/mips/kernel/ftrace.c +0 -0
@@ -304,7 +304,14 @@ void prepare_ftrace_return(unsigned long
 *parent_ra_addr, unsigned long self_ra,
                return;
        }
 

Quoted 12 lines

Quoted 10 lines
arch/mips/kernel/ftrace.c +4 -1
-       trace.func = self_ra;
+       /*
+        * Get the recorded ip of the current mcount calling site in the
+        * __mcount_loc section, which will be used to filter the function
+        * entries configured through the tracing/set_graph_function
interface.
+ */
+
+ insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
Unneeded parens.

ok.

Quoted 4 lines

Quoted 2 lines
+ trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
Here too.

ok.

Thanks,
Wu Zhangjin

Quoted 3 lines

WBR, Sergei

Steven Rostedt

2011-01-20 14:04 UTC #16 lore raw

Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer

In reply to Sergei Shtylyov

Quoted 9 lines · Sergei Shtylyov

Quoted 4 lines
arch/mips/kernel/ftrace.c +0 -0
index bc91e4a..62775d7 100644
[...]
Quoted 4 lines
arch/mips/kernel/ftrace.c +0 -0
@@ -304,7 +304,14 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
  		return;
  	}
 

Quoted 10 lines

arch/mips/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra, · line 313
Quoted 8 lines
arch/mips/kernel/ftrace.c +7 -1
-	trace.func = self_ra;
+	/*
+	 * Get the recorded ip of the current mcount calling site in the
+	 * __mcount_loc section, which will be used to filter the function
+	 * entries configured through the tracing/set_graph_function interface.
+	 */
+
+	insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
Unneeded parens.

agreed

Quoted 4 lines

arch/mips/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra, · line 314
Quoted 2 lines
arch/mips/kernel/ftrace.c +1 -0
+	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
Here too.

I like the parens here. Yes, it is basic math precedence, but it stands out to a reviewer who has their brain more focused on correct code than thinking about which op has precedence.

Reviewing code that has:

	trace.func = self_ra - MCOUNT_INSN_SIZE * insns;

And as I my mother language reads left to right, my thought process
goes: subtract MCOUNT_INSN_SIZE from self_ra and then times insns... oh wait, times goes first; stop reset, restart... subtract MCOUNT_INSN_SIZE times insns from self_ra. That stop reset and restart of the thought process breaks the train of thought and could waste a lot more time than just the moment it happened. All this is avoided by the parenthesis that automatically trigger the brain to think those go first.

I say, leave these in.

-- Steve

wu zhangjin

author

2011-01-21 09:09 UTC #17 lore raw

Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer

On Thu, Jan 20, 2011 at 10:04 PM, Steven Rostedt <[email protected]> wrote:
[...]

Quoted 28 lines

Quoted 4 lines
Quoted 2 lines
+
+ insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
Unneeded parens.
agreed
Quoted 4 lines
Quoted 2 lines
+ trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
Here too.
I like the parens here. Yes, it is basic math precedence, but it stands out to a reviewer who has their brain more focused on correct code than thinking about which op has precedence.
Reviewing code that has:
trace.func = self_ra - MCOUNT_INSN_SIZE * insns;
And as I my mother language reads left to right, my thought process
goes: subtract MCOUNT_INSN_SIZE from self_ra and then times insns... oh wait, times goes first; stop reset, restart... subtract MCOUNT_INSN_SIZE times insns from self_ra. That stop reset and restart of the thought process breaks the train of thought and could waste a lot more time than just the moment it happened. All this is avoided by the parenthesis that automatically trigger the brain to think those go first.

Quoted 1 line

I say, leave these in.

Yeah, agree ;-)

parens have become one important part of my coding style for it have saved me from being confused by the problems like you mentioned above.

This is similar to using "a=++i" or "i++; a=i;", the latter is longer but clearer, although If we put them together, the former is also clear, but If we encounter "a=++i;" and "a=i++;" at the same time, we may have a headache ;-)

Will resend it asap with your feedback.

Thanks,
Wu Zhangjin

Quoted 5 lines

-- Steve