lkml.org 
[lkml]   [2013]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] arm: make return_address available for ARM_UNWIND
From
Date
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 */




\
 
 \ /
  Last update: 2013-01-11 23:41    [W:0.060 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site