Messages in this thread | | | Subject | Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND | From | Steven Rostedt <> | Date | Fri, 11 Jan 2013 17:04:15 -0500 |
| |
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 */
| |