Re: [PATCH] ftrace: mcount record based dynamic tracing for ARM

Re: [PATCH] ftrace: mcount record based dynamic tracing for ARM

[Steven Rostedt]

On Thu, 20 Nov 2008, Jim Radford wrote:
> commit 3b17e9f39304e266edf8ef05c4a01e31c6ecbf5c
> Author: Jim Radford <radford@galvanix.com>
> Date:   Thu Nov 20 19:48:39 2008 -0800
> 
>     ftrace: enable dynamic ftrace for arm
>     
>     Update to the latest api and syncing functions with the x86 versions.

Again, Signed-off-by: is missing.

> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 9722f8b..1b4162c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -16,7 +16,9 @@ config ARM
>  	select HAVE_ARCH_KGDB
>  	select HAVE_KPROBES if (!XIP_KERNEL)
>  	select HAVE_KRETPROBES if (HAVE_KPROBES)
> -	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> +	select HAVE_FTRACE_MCOUNT_RECORD
> +	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)

Russell mentioned something about the code not being compatible with 
Thumb2, is the above if statement enough?

> +	select HAVE_FUNCTION_TRACER
>  	select HAVE_GENERIC_DMA_COHERENT
>  	help
>  	  The ARM series is a line of low-power-consumption RISC chip designs
> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> index 39c8bc1..877c0d4 100644
> --- a/arch/arm/include/asm/ftrace.h
> +++ b/arch/arm/include/asm/ftrace.h
> @@ -7,6 +7,19 @@
>  
>  #ifndef __ASSEMBLY__
>  extern void mcount(void);
> +
> +static inline unsigned long ftrace_call_adjust(unsigned long addr)
> +{
> +	return addr;
> +}
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE
> +
> +struct dyn_arch_ftrace {
> +	/* No extra data needed for x86 */

Do you mean: /* No extra data needed for ARM */

;-)

> +};
> +
> +#endif /*  CONFIG_DYNAMIC_FTRACE */
>  #endif
>  
>  #endif
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 06269ea..06216af 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -104,14 +104,7 @@ ENDPROC(ret_from_fork)
>  #ifdef CONFIG_FUNCTION_TRACER
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  ENTRY(mcount)
> -	stmdb sp!, {r0-r3, lr}
> -	mov r0, lr
> -	sub r0, r0, #MCOUNT_INSN_SIZE
> -
> -	.globl mcount_call
> -mcount_call:
> -	bl ftrace_stub
> -	ldmia sp!, {r0-r3, pc}
> +	mov pc, lr

This looks OK. The new mcount stub for dynamic ftrace is a simple return.
It has been a while since I've programmed ARM (7 years ago), but I'm 
assuming that loading the program counter with the link register is 
a simple return.

>  
>  ENTRY(ftrace_caller)
>  	stmdb sp!, {r0-r3, lr}
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index 6c90479..d63b5a6 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -23,13 +23,13 @@
>  static unsigned long bl_insn;
>  static const unsigned long NOP = 0xe1a00000; /* mov r0, r0 */
>  
> -unsigned char *ftrace_nop_replace(void)
> +static unsigned char *ftrace_nop_replace(void)
>  {
>  	return (char *)&NOP;
>  }
>  
>  /* construct a branch (BL) instruction to addr */
> -unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
> +static unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
>  {
>  	long offset;
>  
> @@ -46,7 +46,7 @@ unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
>  	return (unsigned char *)&bl_insn;
>  }
>  
> -int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
> +static int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
>  		       unsigned char *new_code)
>  {

The ftrace_modify_code could use a bit of clean up. Unless there is some
strange reason that it can not use the probe_kernel_ functions, it needs
to be updated. That approach is the most robust.  Here's an example of 
what it should look like:

{
	unsigned char replaced[MCOUNT_INSN_SIZE];

	/*
	 * Note: Due to modules and __init, code can
	 *  disappear and change, we need to protect against faulting
	 *  as well as code changing. We do this by using the
	 *  probe_kernel_* functions.
	 *
	 * No real locking needed, this code is run through
	 * kstop_machine, or before SMP starts.
	 */

	/* read the text we want to modify */
	if (probe_kernel_read(replaced, (void *)pc, MCOUNT_INSN_SIZE))
		return -EFAULT;

	/* Make sure it is what we expect it to be */
	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
		return -EINVAL;

	/* replace the text with the new text */
	if (probe_kernel_write((void *)pc, new_code, MCOUNT_INSN_SIZE))
		return -EPERM;

	flush_icache_range(pc, pc + MCOUNT_INSN_SIZE);

	return 0;
}

And the new design expects special return values:

 -EFAULT: error reading the address
 -EINVAL: what was at the address was not what we expected
 -EPERM:  could not write to the address

-- Steve


>  	unsigned long err = 0, replaced = 0, old, new;
> @@ -82,22 +82,46 @@ int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
>  	return err;
>  }
>  
> +int ftrace_make_nop(struct module *mod,
> +		    struct dyn_ftrace *rec, unsigned long addr)
> +{
> +	unsigned char *new, *old;
> +	unsigned long ip = rec->ip;
> +
> +	old = ftrace_call_replace(ip, addr);
> +	new = ftrace_nop_replace();
> +
> +	return ftrace_modify_code(rec->ip, old, new);
> +}
> +
> +int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> +{
> +	unsigned char *new, *old;
> +	unsigned long ip = rec->ip;
> +
> +	old = ftrace_nop_replace();
> +	new = ftrace_call_replace(ip, addr);
> +
> +	return ftrace_modify_code(rec->ip, old, new);
> +}
> +
>  int ftrace_update_ftrace_func(ftrace_func_t func)
>  {
> +	unsigned long ip = (unsigned long)(&ftrace_call);
> +	unsigned char old[MCOUNT_INSN_SIZE], *new;
>  	int ret;
> -	unsigned long pc, old;
> -	unsigned char *new;
>  
> -	pc = (unsigned long)&ftrace_call;
> -	memcpy(&old, &ftrace_call, MCOUNT_INSN_SIZE);
> -	new = ftrace_call_replace(pc, (unsigned long)func);
> -	ret = ftrace_modify_code(pc, (unsigned char *)&old, new);
> +	memcpy(old, &ftrace_call, sizeof(old));
> +	new = ftrace_call_replace(ip, (unsigned long)func);
> +	ret = ftrace_modify_code(ip, (unsigned char *)&old, new);
> +
>  	return ret;
>  }
>  
>  /* run from kstop_machine */
>  int __init ftrace_dyn_arch_init(void *data)
>  {
> -	ftrace_mcount_set(data);
> +	/* The return code is retured via data */
> +	*(unsigned long *)data = 0;
>  	return 0;
>  }

Re: [PATCH] ftrace: mcount record based dynamic tracing for ARM

[Russell King - ARM Linux]

On Fri, Nov 21, 2008 at 09:27:17AM -0500, Steven Rostedt wrote:
> On Thu, 20 Nov 2008, Jim Radford wrote:
> > -	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> > +	select HAVE_FTRACE_MCOUNT_RECORD
> > +	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)
> 
> Russell mentioned something about the code not being compatible with 
> Thumb2, is the above if statement enough?

I think I said (or should've said) "upcoming Thumb 2" - it's not yet in
the kernel but there's a patch series floating around for it.  We've
started on merging some of the pre-requisits, and it will mean that
the instruction length is no longer constant.  (It may be a 16bit or
32bit instruction.)

I suspect that ftrace won't be able to handle that, so it may have to
depend on !THUMB2_KERNEL for the time being.

> > diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> > index 06269ea..06216af 100644
> > --- a/arch/arm/kernel/entry-common.S
> > +++ b/arch/arm/kernel/entry-common.S
> > @@ -104,14 +104,7 @@ ENDPROC(ret_from_fork)
> >  #ifdef CONFIG_FUNCTION_TRACER
> >  #ifdef CONFIG_DYNAMIC_FTRACE
> >  ENTRY(mcount)
> > -	stmdb sp!, {r0-r3, lr}
> > -	mov r0, lr
> > -	sub r0, r0, #MCOUNT_INSN_SIZE
> > -
> > -	.globl mcount_call
> > -mcount_call:
> > -	bl ftrace_stub
> > -	ldmia sp!, {r0-r3, pc}
> > +	mov pc, lr
> 
> This looks OK. The new mcount stub for dynamic ftrace is a simple return.
> It has been a while since I've programmed ARM (7 years ago), but I'm 
> assuming that loading the program counter with the link register is 
> a simple return.

Yes, that's all that's required.

Re: [PATCH] ftrace: mcount record based dynamic tracing for ARM

[Steven Rostedt]

On Fri, 21 Nov 2008, Russell King - ARM Linux wrote:

> On Fri, Nov 21, 2008 at 09:27:17AM -0500, Steven Rostedt wrote:
> > On Thu, 20 Nov 2008, Jim Radford wrote:
> > > -	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> > > +	select HAVE_FTRACE_MCOUNT_RECORD
> > > +	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)
> > 
> > Russell mentioned something about the code not being compatible with 
> > Thumb2, is the above if statement enough?
> 
> I think I said (or should've said) "upcoming Thumb 2" - it's not yet in
> the kernel but there's a patch series floating around for it.  We've
> started on merging some of the pre-requisits, and it will mean that
> the instruction length is no longer constant.  (It may be a 16bit or
> 32bit instruction.)
> 
> I suspect that ftrace won't be able to handle that, so it may have to
> depend on !THUMB2_KERNEL for the time being.

Actually it depends on how the compiler adds the mcount call. That's all 
that ftrace touches. The call to mcount. If all callers to mcount stay as 
32 bit, then it may still work.

-- Steve

Re: [PATCH] ftrace: mcount record based dynamic tracing for ARM

[Jim Radford]

On Fri, Nov 21, 2008 at 03:38:27PM +0000, Russell King - ARM Linux wrote:
> On Fri, Nov 21, 2008 at 09:27:17AM -0500, Steven Rostedt wrote:
> > On Thu, 20 Nov 2008, Jim Radford wrote:
> > > -	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> > > +	select HAVE_FTRACE_MCOUNT_RECORD
> > > +	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)

> > Russell mentioned something about the code not being compatible
> > with Thumb2, is the above if statement enough?

> We've started on merging some of the pre-requisits, and it will mean
> that the instruction length is no longer constant.  (It may be a
> 16bit or 32bit instruction.)

The only instruction that matters for arm is "bl <func>" since that's
what's emitted by gcc to call mcount().  I suspect thumb will be easy
to support.  <func> isn't known when the file is compiled, so I assume
in that case the assembler will have to leave at least 4 bytes (even
in thumb) in case mcount() gets linked far away.

I haven't looked at the return tracing code yet.  That might be harder
to support, but given x86 works, I suspect it'll be doable.

-Jim