[PATCH 0/5] Misc updates for Ftrace of MIPS

[PATCH 0/5] Misc updates for Ftrace of MIPS

[Wu Zhangjin]

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

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

[Wu Zhangjin]

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 <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 5a84a1f..635c1dc 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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

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

[Ralf Baechle]

Applied,

  Ralf

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

[Wu Zhangjin]

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 <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 5a84a1f..635c1dc 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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

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

[wu zhangjin]

Sorry, please ignore this one.

On Thu, Jan 20, 2011 at 3:28 AM, Wu Zhangjin <wuzhangjin@gmail.com> wrote:
> 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 <wuzhangjin@gmail.com>
> ---
>  arch/mips/kernel/ftrace.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 5a84a1f..635c1dc 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -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
>
>

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

[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 <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   54 ++++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 635c1dc..5970286 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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

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

[wu zhangjin]

And ignore this one too.

On Thu, Jan 20, 2011 at 3:28 AM, Wu Zhangjin <wuzhangjin@gmail.com> wrote:
> 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 <wuzhangjin@gmail.com>
> ---
>  arch/mips/kernel/ftrace.c |   54 ++++++++++++++++++++++----------------------
>  1 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 635c1dc..5970286 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -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
>
>

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

[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 <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   54 ++++++++++++++++++++++----------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 635c1dc..5970286 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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

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

[Ralf Baechle]

On Thu, Jan 20, 2011 at 03:28:29AM +0800, Wu Zhangjin wrote:

> @@ -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

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

[wu zhangjin]

On Mon, Jan 24, 2011 at 9:57 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Thu, Jan 20, 2011 at 03:28:29AM +0800, Wu Zhangjin wrote:
>
>> @@ -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 */
>        }

Yeah, will check if the following statement works:

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

This check may be faster and clearer ;-)

>
> 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

>
>  Ralf
>

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

[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 <xzhong86@163.com>
Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   52 +++++++++++++++++++++-----------------------
 1 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 5970286..40ef34c 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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:
-- 
1.7.1

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

[Ralf Baechle]

Applied.  Thanks,

  Ralf

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

[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 <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   70 ++++++++++++++++++++++++--------------------
 1 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 40ef34c..bc91e4a 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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);
 }
 
-- 
1.7.1

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

[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 <xzhong86@163.com>
Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index bc91e4a..62775d7 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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)) {
-- 
1.7.1

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

[Sergei Shtylyov]

Hello.

On 19-01-2011 22:28, Wu Zhangjin wrote:

> 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?

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

> Reported-by: Zhiping Zhong<xzhong86@163.com>
> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
> ---
>   arch/mips/kernel/ftrace.c |   11 +++++++++--
>   1 files changed, 9 insertions(+), 2 deletions(-)

> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index bc91e4a..62775d7 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
[...]
> @@ -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.

> +	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);

    Here too.

WBR, Sergei

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

[wu zhangjin]

On Thu, Jan 20, 2011 at 7:03 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 19-01-2011 22:28, Wu Zhangjin wrote:
>
>> 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 ;-)

>
>> This fixes it via calculating the right recorded ip in the __mcount_loc
>> section and assign it to trace.func.
>
>> Reported-by: Zhiping Zhong<xzhong86@163.com>
>> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
>> ---
>>  arch/mips/kernel/ftrace.c |   11 +++++++++--
>>  1 files changed, 9 insertions(+), 2 deletions(-)
>
>> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
>> index bc91e4a..62775d7 100644
>> --- a/arch/mips/kernel/ftrace.c
>> +++ b/arch/mips/kernel/ftrace.c
>
> [...]
>>
>> @@ -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.

ok.

>
>> +       trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
>
>   Here too.

ok.

Thanks,
Wu Zhangjin

>
> WBR, Sergei
>

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

[Steven Rostedt]

On Thu, 2011-01-20 at 14:03 +0300, Sergei Shtylyov wrote:

> > diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> > index bc91e4a..62775d7 100644
> > --- a/arch/mips/kernel/ftrace.c
> > +++ b/arch/mips/kernel/ftrace.c
> [...]
> > @@ -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.

agreed

> 
> > +	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

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

[wu zhangjin]

On Thu, Jan 20, 2011 at 10:04 PM, Steven Rostedt <srostedt@redhat.com> wrote:
[...]
>> > +
>> > +   insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
>>
>>     Unneeded parens.
>
> agreed
>
>>
>> > +   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.

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

>
> -- Steve
>
>
>