[RFC 0/2] Fixes the modules support of dynamic function tracer for MIPS

[RFC 0/2] Fixes the modules support of dynamic function tracer for MIPS

[Wu Zhangjin]

From: Wu Zhangjin <wuzhangjin@gmail.com>

Hi, all

Currently, our in_module() defined in arch/mips/kernel/ftrace.c and
scripts/recordmcount.pl for MIPS only considers the module and the core kernel
have different address space and long call is assumed to be used.

But as David pointed before, the module may be in the same space as the core
kernel, therefore, with the current implementation, dynamic function tracer for
MIPS will not work for that situation.

This patchset is created to fix it.

As we know, to jump from an address to _mcount, for _mcount is in kernel space,
if the address is also in kernel space, then, no long call is needed(for the
"jal" can jump to a place whose offset is smaller than 2^28=256MB and
no kernel image is possible to be bigger than 256MB), otherwise, (only
consider the address passed by ftrace_make_{nop,call}), to jump from
the address to _mcount, long call via "jalr" is needed:

if (in_kernel_space(addr)) {
	jal _mcount;
} else {
	load the address of _mcount to a register
	jalr <register>
}

Now, after implementing in_kernel_space() in the 1st patch, that
situation(module and core kernel are in the same address space) is also covered.

But the 1st patch is not enough to fix the whole problem, we also need to
record the right calling site(for long call, not really the calling site, but
the position for loading the address to the register) for the module in that
situation. Because no long call is needed for that situation, to get the
calling site to _mcount, we need to search the R_MIPS_26 like the kernel, the
2nd patch does it.

Regards,
	Wu Zhangjin

Wu Zhangjin (2):
  MIPS: tracing/ftrace: Replace in_module() with a generic
    in_kernel_space()
  MIPS: tracing/ftrace: Fixes mcount_regex for modules

 arch/mips/kernel/ftrace.c |   66 ++++++++++++++++++++++++--------------------
 scripts/recordmcount.pl   |   46 +++++++++++++++++++-----------
 2 files changed, 65 insertions(+), 47 deletions(-)

[RFC 1/2] MIPS: tracing/ftrace: Replace in_module() with a generic in_kernel_space()

[Wu Zhangjin]

From: Wu Zhangjin <wuzhangjin@gmail.com>

The module space may be the same as the kernel space sometimes(with
related kernel config and gcc options), but the current in_module()
assume they are always different, so, it should be fixed.

As we know, for the limitation of the fixed 32bit long instruction of
MIPS, the "jal target" can only jump to an address whose offset from the
jal instruction is smaller than 2^28=256M, which means if the address is
in kernel space(the kernel image should be always smaller than 256M), no
long call is needed to jump from the address to _mcount, and further, if
the module use the same space as kernel space, the situation for module
will be the same as the kernel. but currently for MIPS, in most of the
situations, module has different space(0xc0000000) from the kernel
space(0x80000000) and the offset is bigger than 256M, a register is
needed to load the address of _mcount and another instruction "jal
<register>" is needed to jump from an address to _mcount.

The above can be explained as:

if (in_kernel_space(addr)) {
	jal _mcount;
} else {
	load the address of _mcount to a register
	jalr <register>
}

As we can see, the above explanation covers all of the situations well
and reflect the reality, so, we decide to add a new in_kernel_space()
instead of the old in_module().

Based on core_kernel_text() from kernel/extable.c, in_kernel_space() is
easily to be added. Because all of the functions in the init sections is
anotated with notrace, so, differ from core_kernel_text(),
in_kernel_space() doesn't care about init section.

With this new in_kernel_space(), the ftrace_make_{nop,call} can be
explained as following.

ftrace_make_call()					ftrace_make_nop()

if (in_kernel_space(addr)) {
	jal _mcount;	(jal ftrace_caller)	  <-->	nop
} else {
	load the address of _mcount to a register <-->	b label
	jalr <register>
	label:
}

Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
---
 arch/mips/kernel/ftrace.c |   66 ++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 65f1949..6ff5a54 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 don't call
+ * init_kernel_text() for Ftrace don'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, long call is not needed, otherwise, it 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,12 @@ 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;
+	/*
+	 * If ip is in kernel space, long call is not needed, otherwise, it is
+	 * needed.
+	 */
+	new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
+		insn_lui_v1_hi16_mcount;
 
 	return ftrace_modify_code(ip, new);
 }
@@ -200,11 +204,13 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
 	int faulted;
 
 	/*
-	 * 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)
+	 * If self_addr is in kernel space, long call is not needed, only need
+	 * to move ip after the instruction "move ra, at"(offset is 16),
+	 * otherwise, long call is needed, need to move ip from calling site of
+	 * mcount after the instruction "lui v1, hi_16bit_of_mcount"(offset is
+	 * 24).
 	 */
-	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

[RFC 2/2] MIPS: tracing/ftrace: Fixes mcount_regex for modules

[Wu Zhangjin]

From: Wu Zhangjin <wuzhangjin@gmail.com>

In some situations(with related kernel config and gcc options), the
modules may have the same address space as the core kernel space, so
mcount_regex for modules should also match R_MIPS_26.

Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
---
 scripts/recordmcount.pl |   46 +++++++++++++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index e67f054..e9c1a0f 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -299,14 +299,33 @@ if ($arch eq "x86_64") {
     $cc .= " -m64";
     $objcopy .= " -O elf64-sparc";
 } elsif ($arch eq "mips") {
-    # To enable module support, we need to enable the -mlong-calls option
-    # of gcc for module, after using this option, we can not get the real
-    # offset of the calling to _mcount, but the offset of the lui
-    # instruction or the addiu one. herein, we record the address of the
-    # first one, and then we can replace this instruction by a branch
-    # instruction to jump over the profiling function to filter the
-    # indicated functions, or swith back to the lui instruction to trace
-    # them, which means dynamic tracing.
+    # <For kernel>
+    # To disable tracing, just replace "jal _mcount" with nop;
+    # to enable tracing, replace back. so, the offset 14 is
+    # needed to be recorded.
+    #
+    #     10:   03e0082d        move    at,ra
+    #     14:   0c000000        jal     0
+    #                    14: R_MIPS_26   _mcount
+    #                    14: R_MIPS_NONE *ABS*
+    #                    14: R_MIPS_NONE *ABS*
+    #     18:   00020021        nop
+    #
+    # <For module>
+    #
+    # If no long call(-mlong-calls), the same to kernel.
+    #
+    # If the module space differs from the kernel space, long
+    # call is needed, as a result, the address of _mcount is
+    # needed to be recorded in a register and then jump from
+    # module space to kernel space via "jalr <register>". To
+    # disable tracing, "jalr <register>" can be replaced by
+    # nop; to enable tracing, replace it back. Since the
+    # offset of "jalr <register>" is not easy to be matched,
+    # the offset of the 1st _mcount below is recorded and to
+    # disable tracing, "lui v1, 0x0" is substituted with "b
+    # label", which jumps over "jalr <register>"; to enable
+    # tracing, replace it back.
     #
     #       c:	3c030000 	lui	v1,0x0
     #			c: R_MIPS_HI16	_mcount
@@ -318,19 +337,12 @@ if ($arch eq "x86_64") {
     #			10: R_MIPS_NONE	*ABS*
     #      14:	03e0082d 	move	at,ra
     #      18:	0060f809 	jalr	v1
+    #   label:
     #
-    # for the kernel:
-    #
-    #     10:   03e0082d        move    at,ra
-    #	  14:   0c000000        jal     0 <loongson_halt>
-    #                    14: R_MIPS_26   _mcount
-    #                    14: R_MIPS_NONE *ABS*
-    #                    14: R_MIPS_NONE *ABS*
-    #	 18:   00020021        nop
     if ($is_module eq "0") {
 	    $mcount_regex = "^\\s*([0-9a-fA-F]+): R_MIPS_26\\s+_mcount\$";
     } else {
-	    $mcount_regex = "^\\s*([0-9a-fA-F]+): R_MIPS_HI16\\s+_mcount\$";
+	    $mcount_regex = "^\\s*([0-9a-fA-F]+): R_MIPS_(HI16|26)\\s+_mcount\$";
     }
     $objdump .= " -Melf-trad".$endian."mips ";
 
-- 
1.7.1

Re: [RFC 2/2] MIPS: tracing/ftrace: Fixes mcount_regex for modules

[David Daney]

On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
> From: Wu Zhangjin<wuzhangjin@gmail.com>
>
> In some situations(with related kernel config and gcc options), the
> modules may have the same address space as the core kernel space, so
> mcount_regex for modules should also match R_MIPS_26.
>

I think Steve is rewriting this bit to be a pure C program.  Is this 
file even used anymore?  If so for how long?

David Daney

> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
> ---
>   scripts/recordmcount.pl |   46 +++++++++++++++++++++++++++++-----------------
>   1 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index e67f054..e9c1a0f 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -299,14 +299,33 @@ if ($arch eq "x86_64") {
>       $cc .= " -m64";
>       $objcopy .= " -O elf64-sparc";
>   } elsif ($arch eq "mips") {
> -    # To enable module support, we need to enable the -mlong-calls option
> -    # of gcc for module, after using this option, we can not get the real
> -    # offset of the calling to _mcount, but the offset of the lui
> -    # instruction or the addiu one. herein, we record the address of the
> -    # first one, and then we can replace this instruction by a branch
> -    # instruction to jump over the profiling function to filter the
> -    # indicated functions, or swith back to the lui instruction to trace
> -    # them, which means dynamic tracing.
> +    #<For kernel>
> +    # To disable tracing, just replace "jal _mcount" with nop;
> +    # to enable tracing, replace back. so, the offset 14 is
> +    # needed to be recorded.
> +    #
> +    #     10:   03e0082d        move    at,ra
> +    #     14:   0c000000        jal     0
> +    #                    14: R_MIPS_26   _mcount
> +    #                    14: R_MIPS_NONE *ABS*
> +    #                    14: R_MIPS_NONE *ABS*
> +    #     18:   00020021        nop
> +    #
> +    #<For module>
> +    #
> +    # If no long call(-mlong-calls), the same to kernel.
> +    #
> +    # If the module space differs from the kernel space, long
> +    # call is needed, as a result, the address of _mcount is
> +    # needed to be recorded in a register and then jump from
> +    # module space to kernel space via "jalr<register>". To
> +    # disable tracing, "jalr<register>" can be replaced by
> +    # nop; to enable tracing, replace it back. Since the
> +    # offset of "jalr<register>" is not easy to be matched,
> +    # the offset of the 1st _mcount below is recorded and to
> +    # disable tracing, "lui v1, 0x0" is substituted with "b
> +    # label", which jumps over "jalr<register>"; to enable
> +    # tracing, replace it back.
>       #
>       #       c:	3c030000 	lui	v1,0x0
>       #			c: R_MIPS_HI16	_mcount
> @@ -318,19 +337,12 @@ if ($arch eq "x86_64") {
>       #			10: R_MIPS_NONE	*ABS*
>       #      14:	03e0082d 	move	at,ra
>       #      18:	0060f809 	jalr	v1
> +    #   label:
>       #
> -    # for the kernel:
> -    #
> -    #     10:   03e0082d        move    at,ra
> -    #	  14:   0c000000        jal     0<loongson_halt>
> -    #                    14: R_MIPS_26   _mcount
> -    #                    14: R_MIPS_NONE *ABS*
> -    #                    14: R_MIPS_NONE *ABS*
> -    #	 18:   00020021        nop
>       if ($is_module eq "0") {
>   	    $mcount_regex = "^\\s*([0-9a-fA-F]+): R_MIPS_26\\s+_mcount\$";
>       } else {
> -	    $mcount_regex = "^\\s*([0-9a-fA-F]+): R_MIPS_HI16\\s+_mcount\$";
> +	    $mcount_regex = "^\\s*([0-9a-fA-F]+): R_MIPS_(HI16|26)\\s+_mcount\$";
>       }
>       $objdump .= " -Melf-trad".$endian."mips ";
>

Re: [RFC 1/2] MIPS: tracing/ftrace: Replace in_module() with a generic in_kernel_space()

[David Daney]

On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
> From: Wu Zhangjin<wuzhangjin@gmail.com>
>
> The module space may be the same as the kernel space sometimes(with
> related kernel config and gcc options), but the current in_module()
> assume they are always different, so, it should be fixed.
>
> As we know, for the limitation of the fixed 32bit long instruction of
> MIPS, the "jal target" can only jump to an address whose offset from the
> jal instruction is smaller than 2^28=256M, which means if the address is
> in kernel space(the kernel image should be always smaller than 256M), no
> long call is needed to jump from the address to _mcount, and further, if
> the module use the same space as kernel space, the situation for module
> will be the same as the kernel. but currently for MIPS, in most of the
> situations, module has different space(0xc0000000) from the kernel
> space(0x80000000) and the offset is bigger than 256M, a register is
> needed to load the address of _mcount and another instruction "jal
> <register>" is needed to jump from an address to _mcount.
>
> The above can be explained as:
>
> if (in_kernel_space(addr)) {
> 	jal _mcount;
> } else {
> 	load the address of _mcount to a register
> 	jalr<register>
> }
>

Why can't we just do code examination to determine which case we are 
patching?  There is a very limited and fixed set of instruction 
sequences that GCC will emit for _mcount calls.  We can easily check for 
all of them.

The mcount call sequence unconditionally emits:

	MOVE $1,$31

In the JALR case (for -mlong-call), the instruction immediately before 
this will have the target register be '$3'.

I would rather directly check which case we are patching rather than 
trying to infer it from the address.

Is there any case where a 'stock' kernel currently fails?  If not, this 
patch seems like unneeded code churn.



> As we can see, the above explanation covers all of the situations well
> and reflect the reality, so, we decide to add a new in_kernel_space()
> instead of the old in_module().
>
> Based on core_kernel_text() from kernel/extable.c, in_kernel_space() is
> easily to be added. Because all of the functions in the init sections is
> anotated with notrace, so, differ from core_kernel_text(),
> in_kernel_space() doesn't care about init section.
>
> With this new in_kernel_space(), the ftrace_make_{nop,call} can be
> explained as following.
>
> ftrace_make_call()					ftrace_make_nop()
>
> if (in_kernel_space(addr)) {
> 	jal _mcount;	(jal ftrace_caller)	<-->	nop
> } else {
> 	load the address of _mcount to a register<-->	b label
> 	jalr<register>
> 	label:
> }
>
> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
> ---
>   arch/mips/kernel/ftrace.c |   66 ++++++++++++++++++++++++--------------------
>   1 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 65f1949..6ff5a54 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 don't call
> + * init_kernel_text() for Ftrace don'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, long call is not needed, otherwise, it 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,12 @@ 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;
> +	/*
> +	 * If ip is in kernel space, long call is not needed, otherwise, it is
> +	 * needed.
> +	 */
> +	new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
> +		insn_lui_v1_hi16_mcount;
>
>   	return ftrace_modify_code(ip, new);
>   }
> @@ -200,11 +204,13 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
>   	int faulted;
>
>   	/*
> -	 * 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)
> +	 * If self_addr is in kernel space, long call is not needed, only need
> +	 * to move ip after the instruction "move ra, at"(offset is 16),
> +	 * otherwise, long call is needed, need to move ip from calling site of
> +	 * mcount after the instruction "lui v1, hi_16bit_of_mcount"(offset is
> +	 * 24).
>   	 */
> -	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}

Re: [RFC 2/2] MIPS: tracing/ftrace: Fixes mcount_regex for modules

[Steven Rostedt]

On Fri, 2010-10-22 at 14:10 -0700, David Daney wrote:
> On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
> > From: Wu Zhangjin<wuzhangjin@gmail.com>
> >
> > In some situations(with related kernel config and gcc options), the
> > modules may have the same address space as the core kernel space, so
> > mcount_regex for modules should also match R_MIPS_26.
> >
> 
> I think Steve is rewriting this bit to be a pure C program.  Is this 
> file even used anymore?  If so for how long?

It's already in mainline, but is only supported for x86 for now. Until
we verify that it works for other archs (which is up to you guys to
verify) the script will still be used.

Also, I did not write it, John Reiser did. I just cleaned it up a bit
and got it working with the build system.

-- Steve

Re: [RFC 1/2] MIPS: tracing/ftrace: Replace in_module() with a generic in_kernel_space()

[wu zhangjin]

On Sat, Oct 23, 2010 at 5:31 AM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
>>
>> From: Wu Zhangjin<wuzhangjin@gmail.com>
>>
>> The module space may be the same as the kernel space sometimes(with
>> related kernel config and gcc options), but the current in_module()
>> assume they are always different, so, it should be fixed.
>>
>> As we know, for the limitation of the fixed 32bit long instruction of
>> MIPS, the "jal target" can only jump to an address whose offset from the
>> jal instruction is smaller than 2^28=256M, which means if the address is
>> in kernel space(the kernel image should be always smaller than 256M), no
>> long call is needed to jump from the address to _mcount, and further, if
>> the module use the same space as kernel space, the situation for module
>> will be the same as the kernel. but currently for MIPS, in most of the
>> situations, module has different space(0xc0000000) from the kernel
>> space(0x80000000) and the offset is bigger than 256M, a register is
>> needed to load the address of _mcount and another instruction "jal
>> <register>" is needed to jump from an address to _mcount.
>>
>> The above can be explained as:
>>
>> if (in_kernel_space(addr)) {
>>        jal _mcount;
>> } else {
>>        load the address of _mcount to a register
>>        jalr<register>
>> }
>>
>
> Why can't we just do code examination to determine which case we are
> patching? There is a very limited and fixed set of instruction sequences
> that GCC will emit for _mcount calls.  We can easily check for all of them.
>
> The mcount call sequence unconditionally emits:
>
>        MOVE $1,$31
>
> In the JALR case (for -mlong-call), the instruction immediately before this
> will have the target register be '$3'.
>

Yes, we can, but we care about the performance here:

The current in_kernel_space() only need to check the address itself
but checking the code at (address-4) need an extra memory access .
Because ftrace_make_{nop,call} may be called to cope with a large
number of _mcount calling sites, considering the low frequency of the
MIPS, using a working and light-weight method here is more important,
so ... why not checking the address directly and the point is it works
well.

> I would rather directly check which case we are patching rather than trying
> to infer it from the address.
>
> Is there any case where a 'stock' kernel currently fails?  If not, this
> patch seems like unneeded code churn.

Hmm, not found such a failure yet, but theoretically the failure may
happen for the old implementation(The in_module() in
arch/mips/kernel/ftrace.c) had the assumption of module would always
be compiled with -mlong-calls, but the method(in_kernel_space) used in
this patch discards the assumption and therefore it is more generic.

Thanks,
Wu Zhangjin

>
>
>
>> As we can see, the above explanation covers all of the situations well
>> and reflect the reality, so, we decide to add a new in_kernel_space()
>> instead of the old in_module().
>>
>> Based on core_kernel_text() from kernel/extable.c, in_kernel_space() is
>> easily to be added. Because all of the functions in the init sections is
>> anotated with notrace, so, differ from core_kernel_text(),
>> in_kernel_space() doesn't care about init section.
>>
>> With this new in_kernel_space(), the ftrace_make_{nop,call} can be
>> explained as following.
>>
>> ftrace_make_call()                                      ftrace_make_nop()
>>
>> if (in_kernel_space(addr)) {
>>        jal _mcount;    (jal ftrace_caller)     <-->    nop
>> } else {
>>        load the address of _mcount to a register<-->   b label
>>        jalr<register>
>>        label:
>> }
>>
>> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
>> ---
>>  arch/mips/kernel/ftrace.c |   66
>> ++++++++++++++++++++++++--------------------
>>  1 files changed, 36 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
>> index 65f1949..6ff5a54 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 don't call
>> + * init_kernel_text() for Ftrace don'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, long call is not needed, otherwise,
>> it 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,12 @@ 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;
>> +       /*
>> +        * If ip is in kernel space, long call is not needed, otherwise,
>> it is
>> +        * needed.
>> +        */
>> +       new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
>> +               insn_lui_v1_hi16_mcount;
>>
>>        return ftrace_modify_code(ip, new);
>>  }
>> @@ -200,11 +204,13 @@ unsigned long ftrace_get_parent_addr(unsigned long
>> self_addr,
>>        int faulted;
>>
>>        /*
>> -        * 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)
>> +        * If self_addr is in kernel space, long call is not needed, only
>> need
>> +        * to move ip after the instruction "move ra, at"(offset is 16),
>> +        * otherwise, long call is needed, need to move ip from calling
>> site of
>> +        * mcount after the instruction "lui v1,
>> hi_16bit_of_mcount"(offset is
>> +        * 24).
>>         */
>> -       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}
>
>

Re: [RFC 2/2] MIPS: tracing/ftrace: Fixes mcount_regex for modules

[wu zhangjin]

On Sat, Oct 23, 2010 at 5:31 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2010-10-22 at 14:10 -0700, David Daney wrote:
>> On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
>> > From: Wu Zhangjin<wuzhangjin@gmail.com>
>> >
>> > In some situations(with related kernel config and gcc options), the
>> > modules may have the same address space as the core kernel space, so
>> > mcount_regex for modules should also match R_MIPS_26.
>> >
>>
>> I think Steve is rewriting this bit to be a pure C program.

Good reminder, thanks ;-)

Just found "[GIT PULL][2.6.37] ftrace: C version of recordmcount".

>>  Is this
>> file even used anymore?  If so for how long?
>
> It's already in mainline, but is only supported for x86 for now. Until
> we verify that it works for other archs (which is up to you guys to
> verify) the script will still be used.
>

I will verify it asap.

Thanks & Regards,
Wu Zhangjin

> Also, I did not write it, John Reiser did. I just cleaned it up a bit
> and got it working with the build system.
>
> -- Steve
>
>
>

Re: [RFC 1/2] MIPS: tracing/ftrace: Replace in_module() with a generic in_kernel_space()

[wu zhangjin]

Hi, Ralf

I have sent the [RFC 2/2] patch in another patchset [Add C version of
recordmcount for MIPS], and this [RFC 1/2] is also needed to discard
the old assumption on the module address space, so, could you please
apply it directly? or, should I resend it?

You can get it here:

[RFC,1/2] MIPS: tracing/ftrace: Replace in_module() with a generic
in_kernel_space()
http://patchwork.linux-mips.org/patch/1730/

And before applying it, you need to apply this mini patch at first,
otherwise, there will be conflict.

MIPS: tracing/ftrace: Speedup a little for function graph tracer
http://patchwork.linux-mips.org/patch/1728/

Regards,
Wu zhangjin

On Sat, Oct 23, 2010 at 4:58 AM, Wu Zhangjin <wuzhangjin@gmail.com> wrote:
> From: Wu Zhangjin <wuzhangjin@gmail.com>
>
> The module space may be the same as the kernel space sometimes(with
> related kernel config and gcc options), but the current in_module()
> assume they are always different, so, it should be fixed.
>
> As we know, for the limitation of the fixed 32bit long instruction of
> MIPS, the "jal target" can only jump to an address whose offset from the
> jal instruction is smaller than 2^28=256M, which means if the address is
> in kernel space(the kernel image should be always smaller than 256M), no
> long call is needed to jump from the address to _mcount, and further, if
> the module use the same space as kernel space, the situation for module
> will be the same as the kernel. but currently for MIPS, in most of the
> situations, module has different space(0xc0000000) from the kernel
> space(0x80000000) and the offset is bigger than 256M, a register is
> needed to load the address of _mcount and another instruction "jal
> <register>" is needed to jump from an address to _mcount.
>
> The above can be explained as:
>
> if (in_kernel_space(addr)) {
>        jal _mcount;
> } else {
>        load the address of _mcount to a register
>        jalr <register>
> }
>
> As we can see, the above explanation covers all of the situations well
> and reflect the reality, so, we decide to add a new in_kernel_space()
> instead of the old in_module().
>
> Based on core_kernel_text() from kernel/extable.c, in_kernel_space() is
> easily to be added. Because all of the functions in the init sections is
> anotated with notrace, so, differ from core_kernel_text(),
> in_kernel_space() doesn't care about init section.
>
> With this new in_kernel_space(), the ftrace_make_{nop,call} can be
> explained as following.
>
> ftrace_make_call()                                      ftrace_make_nop()
>
> if (in_kernel_space(addr)) {
>        jal _mcount;    (jal ftrace_caller)       <-->  nop
> } else {
>        load the address of _mcount to a register <-->  b label
>        jalr <register>
>        label:
> }
>
> Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
> ---
>  arch/mips/kernel/ftrace.c |   66 ++++++++++++++++++++++++--------------------
>  1 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 65f1949..6ff5a54 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 don't call
> + * init_kernel_text() for Ftrace don'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, long call is not needed, otherwise, it 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,12 @@ 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;
> +       /*
> +        * If ip is in kernel space, long call is not needed, otherwise, it is
> +        * needed.
> +        */
> +       new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
> +               insn_lui_v1_hi16_mcount;
>
>        return ftrace_modify_code(ip, new);
>  }
> @@ -200,11 +204,13 @@ unsigned long ftrace_get_parent_addr(unsigned long self_addr,
>        int faulted;
>
>        /*
> -        * 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)
> +        * If self_addr is in kernel space, long call is not needed, only need
> +        * to move ip after the instruction "move ra, at"(offset is 16),
> +        * otherwise, long call is needed, need to move ip from calling site of
> +        * mcount after the instruction "lui v1, hi_16bit_of_mcount"(offset is
> +        * 24).
>         */
> -       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
>
>