[PATCH 2/2] arm: make return_address available for ARM_UNWIND

[Steven Rostedt]

On Fri, 2013-01-11 at 17:53 +0900, Keun-O Park wrote:

> Hello Steve,
> 
> Much obliged for your explaining this.
> As your guess, there's another path that uses CALLER_ADDR1 without
> CONFIG_PROVE_LOCKING though both lockdep and ftrace's irqsoff tracer
> is turned on. In this case, ftrace's trace_hardirqs_on/off() would be
> called.
> And, as you said to me, if lockdep is off and ftrace's irqsoff is on,
> ftrace's trace_hardirqs_on/off() would be called.
> I think the proper way to avoid calling CALLER_ADDR1 will be calling
> trace_hardirqs_off_caller() instead of calling stop_critical_timing()
> directly in trace_hardirqs_off(). And, this will suppress the message
> outputs of unwind_frame.

That will break the output for users that have a working CALLER_ADDR1.

> If you think this idea is okay, I will push the patch v2 to you.
> In my test result, it looks it's functionally correct.
> 
> Thanks.
> 
> -- kpark
> 
> 
> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> index f89515a..3552ad9 100644
> --- a/arch/arm/include/asm/ftrace.h
> +++ b/arch/arm/include/asm/ftrace.h
> @@ -32,13 +32,11 @@ extern void ftrace_call_old(void);
> 
>  #ifndef __ASSEMBLY__
> 
> -#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
> +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
>  /*
>   * return_address uses walk_stackframe to do it's work.  If both
>   * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
> - * information.  For this to work in the function tracer many functions would
> - * have to be marked with __notrace.  So for now just depend on
> - * !CONFIG_ARM_UNWIND.
> + * information.
>   */
> 
>  void *return_address(unsigned int);
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 5bbec7b..675fd62 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -9,6 +9,9 @@ ifdef CONFIG_FUNCTION_TRACER
>  CFLAGS_REMOVE_ftrace.o = -pg
>  CFLAGS_REMOVE_insn.o = -pg
>  CFLAGS_REMOVE_patch.o = -pg
> +ifdef CONFIG_ARM_UNWIND
> +CFLAGS_REMOVE_unwind.o = -pg
> +endif

You don't need to add the ifdef CONFIG_ARM_UNWIND. It's just setting a
variable and if the unwind.o gets complied, it will apply the removal of
-pg. If the unwind isn't compiled the variable will just be set but
unused. No harm done.

>  endif
> 
>  CFLAGS_REMOVE_return_address.o = -pg



> --- a/kernel/trace/trace_irqsoff.c
> +++ b/kernel/trace/trace_irqsoff.c
> @@ -483,20 +483,6 @@ inline void print_irqtrace_events(struct task_struct *curr)
>  /*
>   * We are only interested in hardirq on/off events:
>   */
> -void trace_hardirqs_on(void)
> -{
> -	if (!preempt_trace() && irq_trace())
> -		stop_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
> -}
> -EXPORT_SYMBOL(trace_hardirqs_on);
> -
> -void trace_hardirqs_off(void)
> -{
> -	if (!preempt_trace() && irq_trace())
> -		start_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
> -}
> -EXPORT_SYMBOL(trace_hardirqs_off);
> -
>  void trace_hardirqs_on_caller(unsigned long caller_addr)
>  {
>  	if (!preempt_trace() && irq_trace())
> @@ -504,6 +490,12 @@ void trace_hardirqs_on_caller(unsigned long caller_addr)
>  }
>  EXPORT_SYMBOL(trace_hardirqs_on_caller);
> 
> +void trace_hardirqs_on(void)
> +{
> +	trace_hardirqs_on_caller(CALLER_ADDR0);
> +}
> +EXPORT_SYMBOL(trace_hardirqs_on);

This is not equivalent. CALLER_ADDR0 will just give us what disabled the
irqs (spin_lock_irq or local_irq_save), but wont give us who called
that.

If you can't handle CALLER_ADDR1, then define it to NULL or to
CALLER_ADDR0.

What the original output gives:

jbd2/dm--1399    3d...    0us : _raw_spin_lock_irqsave
<-ata_scsi_queuecmd

which shows that the _raw_spin_lock_irqsave which disabled irqs was
called by ata_scsi_queuecmd.

With your patch we get:

jbd2/dm--1407    3d...    0us : trace_hardirqs_off
<-_raw_spin_lock_irqsave

Which shows the internal function "trace_hardirqs_off" which is useless,
as well as that the thing that disabled interrupts was
_raw_spin_lock_irqsave. But we don't get who called
_raw_spin_lock_irqsave.

Thus you just broke the code for those that can handle CALLER_ADDR1.

-- Steve

> +
>  void trace_hardirqs_off_caller(unsigned long caller_addr)
>  {
>  	if (!preempt_trace() && irq_trace())
> @@ -511,6 +503,12 @@ void trace_hardirqs_off_caller(unsigned long caller_addr)
>  }
>  EXPORT_SYMBOL(trace_hardirqs_off_caller);
> 
> +void trace_hardirqs_off(void)
> +{
> +	trace_hardirqs_off_caller(CALLER_ADDR0);
> +}
> +EXPORT_SYMBOL(trace_hardirqs_off);
> +
>  #endif /* CONFIG_PROVE_LOCKING */
>  #endif /*  CONFIG_IRQSOFF_TRACER */