[PATCH 1/2] arm: fix returning wrong CALLER_ADDRx

[PATCH 1/2] arm: fix returning wrong CALLER_ADDRx

[kpark3469]

From: sahara <keun-o.park@windriver.com>

This makes return_address return correct value for ftrace feature.
unwind_frame does not update frame->lr but frame->pc for backtrace.
And, the initialization for data.addr was missing so that wrong value
returned when unwind_frame failed.

Signed-off-by: sahara <keun-o.park@windriver.com>
---
 arch/arm/kernel/return_address.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 8085417..fafedd8 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -26,7 +26,7 @@ static int save_return_addr(struct stackframe *frame, void *d)
 	struct return_address_data *data = d;
 
 	if (!data->level) {
-		data->addr = (void *)frame->lr;
+		data->addr = (void *)frame->pc;
 
 		return 1;
 	} else {
@@ -41,7 +41,8 @@ void *return_address(unsigned int level)
 	struct stackframe frame;
 	register unsigned long current_sp asm ("sp");
 
-	data.level = level + 1;
+	data.level = level + 2;
+	data.addr = NULL;
 
 	frame.fp = (unsigned long)__builtin_frame_address(0);
 	frame.sp = current_sp;
-- 
1.7.1

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

[kpark3469]

From: sahara <keun-o.park@windriver.com>

This fixes a warning saying:

    warning: #warning "TODO: return_address should use unwind tables"

And, this enables return_address using unwind information. If ARM_UNWIND is
selected, unwind_frame in unwind.c will be called in walk_stackframe.

Signed-off-by: sahara <keun-o.park@windriver.com>
---
 arch/arm/include/asm/ftrace.h    |    6 ++----
 arch/arm/kernel/return_address.c |   10 +++-------
 arch/arm/kernel/stacktrace.c     |    3 +++
 3 files changed, 8 insertions(+), 11 deletions(-)

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/return_address.c b/arch/arm/kernel/return_address.c
index fafedd8..f202bc0 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -11,7 +11,7 @@
 #include <linux/export.h>
 #include <linux/ftrace.h>
 
-#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
 #include <linux/sched.h>
 
 #include <asm/stacktrace.h>
@@ -57,17 +57,13 @@ void *return_address(unsigned int level)
 		return NULL;
 }
 
-#else /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) */
-
-#if defined(CONFIG_ARM_UNWIND)
-#warning "TODO: return_address should use unwind tables"
-#endif
+#else /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */
 
 void *return_address(unsigned int level)
 {
 	return NULL;
 }
 
-#endif /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) / else */
+#endif /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */
 
 EXPORT_SYMBOL_GPL(return_address);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 00f79e5..aab144b 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -6,6 +6,9 @@
 
 #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
 /*
+ * If both CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses
+ * unwind information. So for now just depend on !CONFIG_ARM_UNWIND.
+ *
  * Unwind the current stack frame and store the new register values in the
  * structure passed as argument. Unwinding is equivalent to a function return,
  * hence the new PC value rather than LR should be used for backtrace.
-- 
1.7.1

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

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 07:12:29PM +0900, kpark3469@gmail.com wrote:
> -#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.

So what have you done about the issue referred in this comment?  Or do you
believe that fixing warnings (even if they are explicit #warning statements)
is far more important than code being functionally correct?

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

[Steven Rostedt]

On Thu, 2013-01-03 at 20:36 +0900, Keun-O Park wrote:

>         
>         So what have you done about the issue referred in this
>         comment?  Or do you
>         believe that fixing warnings (even if they are explicit
>         #warning statements)
>         is far more important than code being functionally correct?
> 
> I admit that I missed to add notrace to unwind.c.
> Do you think there's more to add?

I think Russell wants a better change log. What was written sounds like
the fix was to remove a warning. It wasn't very clear to how the warning
is no longer relevant because of the new changes.

The old change log:

---
This fixes a warning saying:

    warning: #warning "TODO: return_address should use unwind tables"

And, this enables return_address using unwind information. If ARM_UNWIND is
selected, unwind_frame in unwind.c will be called in walk_stackframe.
---


Maybe you wanted to say something like:

---
Now that the return_address code can safely use unwind tables, fix up
the #ifdef statements to reflect this.
---

Or something similar, if that's what was done.

-- Steve


> 
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 00df012..52ff2d4 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -327,7 +327,7 @@ static int unwind_exec_insn(struct
> unwind_ctrl_block *ctrl)
>   * Unwind a single frame starting with *sp for the symbol at *pc. It
>   * updates the *pc and *sp with the new values.
>   */
> -int unwind_frame(struct stackframe *frame)
> +int notrace unwind_frame(struct stackframe *frame)
>  {
>         unsigned long high, low;
>         const struct unwind_idx *idx;
> 

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

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 08:36:05AM -0500, Steven Rostedt wrote:
> On Thu, 2013-01-03 at 20:36 +0900, Keun-O Park wrote:
> >         So what have you done about the issue referred in this
> >         comment?  Or do you
> >         believe that fixing warnings (even if they are explicit
> >         #warning statements)
> >         is far more important than code being functionally correct?
> > 
> > I admit that I missed to add notrace to unwind.c.
> > Do you think there's more to add?
> 
> I think Russell wants a better change log. What was written sounds like
> the fix was to remove a warning. It wasn't very clear to how the warning
> is no longer relevant because of the new changes.
> 
> The old change log:
> 
> ---
> This fixes a warning saying:
> 
>     warning: #warning "TODO: return_address should use unwind tables"
> 
> And, this enables return_address using unwind information. If ARM_UNWIND is
> selected, unwind_frame in unwind.c will be called in walk_stackframe.
> ---
> 
> 
> Maybe you wanted to say something like:
> 
> ---
> Now that the return_address code can safely use unwind tables, fix up
> the #ifdef statements to reflect this.
> ---
> 
> Or something similar, if that's what was done.

No.  What I want is some evidence that the patch author is not just
removing warnings by patching them away, but has thought about why
the warning is there, and that they've resolved the issues for why
that warning is there - and most importantly why the code is not built
in that configuration.

The unwinder has many functions, none of which is marked in any way.
This is alluded to in this comment:

/*
 * 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.
 */

So, what this means is that the function tracer can have hooks in the
unwinder code.  If one of those hooks is active, then there's the
possibility for recursion.

I see no evidence in either of these patches that this issue has been
addressed in any way (let alone thought about.)  The approach here seems
to be "lets remove the comment, lets change the ifdefs to make the code
build, and lets remove the warning".

That's not acceptable.  We don't fix warnings to make them go away while
effectively hiding the original problem.

> 
> -- Steve
> 
> 
> > 
> > diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> > index 00df012..52ff2d4 100644
> > --- a/arch/arm/kernel/unwind.c
> > +++ b/arch/arm/kernel/unwind.c
> > @@ -327,7 +327,7 @@ static int unwind_exec_insn(struct
> > unwind_ctrl_block *ctrl)
> >   * Unwind a single frame starting with *sp for the symbol at *pc. It
> >   * updates the *pc and *sp with the new values.
> >   */
> > -int unwind_frame(struct stackframe *frame)
> > +int notrace unwind_frame(struct stackframe *frame)
> >  {
> >         unsigned long high, low;
> >         const struct unwind_idx *idx;
> > 

And what about search_index(), unwind_find_origin(), unwind_find_idx(),
unwind_get_byte(), unwind_exec_insn(), pr_debug(), pr_warning()?  Maybe
the compiler currently inlines all those functions, but todays compiler
behaviour is no guarantee of tomorrow's compiler behaviour.  We'd also
hope that list_move() would always be inlined and never traced.

It is possible that the ftrace code has some re-entrancy protection to
prevent the unwinder recursing back into the ftrace code, but that's not
mentioned in this patch... so it could be that needs to be explained.

In summary, from what I can see in the patch, the reason why the ifdefs
are the way they are, and the reason the warning is there has not been
addressed; these patches just seem to be aimed just at removing a #warning
statement to make the warning go away.

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

[Steven Rostedt]

On Thu, 2013-01-03 at 14:13 +0000, Russell King - ARM Linux wrote:

> > 
> > Or something similar, if that's what was done.
> 
> No.  What I want is some evidence that the patch author is not just
> removing warnings by patching them away, but has thought about why
> the warning is there, and that they've resolved the issues for why
> that warning is there - and most importantly why the code is not built
> in that configuration.

OK, I understand you now.

> 
> The unwinder has many functions, none of which is marked in any way.
> This is alluded to in this comment:
> 
> /*
>  * 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.
>  */
> 
> So, what this means is that the function tracer can have hooks in the
> unwinder code.  If one of those hooks is active, then there's the
> possibility for recursion.

Note, since this code was written, the function tracer has much better
recursion protection. If one of the callbacks of the function tracer
were to call return_address() and it triggered another traced function,
the function tracer would simply drop it.

Now this isn't true for function graph tracing, at least not for its
internal use. That would need to be investigated to see if there's calls
there (I don't see any). From what I see, the return_address() is used
by ftrace's CALLER_ADDR1-6. These are used by the latency tracers
(irqsoff, preemptoff) as well as lockdep.

Looking at the code in trace_irqsoff.c (which does both irqsoff and
preemptoff), it doesn't look as if the function graph callbacks use
return_address() at all.

The return_address() is used in the management of tracing where the irqs
and preemption was disabled. Not by the functions called in between.

Looks, like the worse that can happen if we allow using the unwind code
here, is that we will see the functions it uses in the trace output. I
don't see where a recursion bug could happen.

But this would need more investigation to be sure.

> 
> I see no evidence in either of these patches that this issue has been
> addressed in any way (let alone thought about.)  The approach here seems
> to be "lets remove the comment, lets change the ifdefs to make the code
> build, and lets remove the warning".
> 
> That's not acceptable.  We don't fix warnings to make them go away while
> effectively hiding the original problem.

I totally agree with you. But now I'm wondering if the problem exists
even without these patches.

> > 
> > 
> > > 
> > > diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> > > index 00df012..52ff2d4 100644
> > > --- a/arch/arm/kernel/unwind.c
> > > +++ b/arch/arm/kernel/unwind.c
> > > @@ -327,7 +327,7 @@ static int unwind_exec_insn(struct
> > > unwind_ctrl_block *ctrl)
> > >   * Unwind a single frame starting with *sp for the symbol at *pc. It
> > >   * updates the *pc and *sp with the new values.
> > >   */
> > > -int unwind_frame(struct stackframe *frame)
> > > +int notrace unwind_frame(struct stackframe *frame)
> > >  {
> > >         unsigned long high, low;
> > >         const struct unwind_idx *idx;
> > > 
> 
> And what about search_index(), unwind_find_origin(), unwind_find_idx(),
> unwind_get_byte(), unwind_exec_insn(), pr_debug(), pr_warning()?  Maybe
> the compiler currently inlines all those functions, but todays compiler
> behaviour is no guarantee of tomorrow's compiler behaviour.  We'd also
> hope that list_move() would always be inlined and never traced.

Some would definitely be traced. Note, that the "inline" keyword adds
'notrace' now to remove those ambiguous cases.

> 
> It is possible that the ftrace code has some re-entrancy protection to
> prevent the unwinder recursing back into the ftrace code, but that's not
> mentioned in this patch... so it could be that needs to be explained.

Function tracing does, and to the most extent so does function graph
tracing as long as the return_address() isn't used internally. Looking
at the code, I don't see where it is.

> 
> In summary, from what I can see in the patch, the reason why the ifdefs
> are the way they are, and the reason the warning is there has not been
> addressed; these patches just seem to be aimed just at removing a #warning
> statement to make the warning go away.

You're correct that this patch does not solve any of theses issues. Now,
I'm thinking that ftrace has matured where these issues don't exist, and
where they do, it will only cause noise in the trace than anything
serious. But, this needs to be looked deeper to make sure.

-- Steve

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

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 11:03:58AM -0500, Steven Rostedt wrote:
> On Thu, 2013-01-03 at 14:13 +0000, Russell King - ARM Linux wrote:
> > In summary, from what I can see in the patch, the reason why the ifdefs
> > are the way they are, and the reason the warning is there has not been
> > addressed; these patches just seem to be aimed just at removing a #warning
> > statement to make the warning go away.
> 
> You're correct that this patch does not solve any of theses issues. Now,
> I'm thinking that ftrace has matured where these issues don't exist, and
> where they do, it will only cause noise in the trace than anything
> serious. But, this needs to be looked deeper to make sure.

Looking back in the archives, it seems that we had a problem with ftrace
and the unwinder polluting the trace information:

http://lists.arm.linux.org.uk/lurker/message/20090604.201745.1c41ee6c.en.html

There's quite a bit of discussion in that thread about this which details
why we came up with what we have today.

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

[Steven Rostedt]

On Thu, 2013-01-03 at 16:23 +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 03, 2013 at 11:03:58AM -0500, Steven Rostedt wrote:
> > On Thu, 2013-01-03 at 14:13 +0000, Russell King - ARM Linux wrote:
> > > In summary, from what I can see in the patch, the reason why the ifdefs
> > > are the way they are, and the reason the warning is there has not been
> > > addressed; these patches just seem to be aimed just at removing a #warning
> > > statement to make the warning go away.
> > 
> > You're correct that this patch does not solve any of theses issues. Now,
> > I'm thinking that ftrace has matured where these issues don't exist, and
> > where they do, it will only cause noise in the trace than anything
> > serious. But, this needs to be looked deeper to make sure.
> 
> Looking back in the archives, it seems that we had a problem with ftrace
> and the unwinder polluting the trace information:
> 
> http://lists.arm.linux.org.uk/lurker/message/20090604.201745.1c41ee6c.en.html
> 
> There's quite a bit of discussion in that thread about this which details
> why we came up with what we have today.

Thanks for the link. In fact, I see I was even involved :-)

http://lists.arm.linux.org.uk/lurker/message/20090609.213448.b4e4d096.en.html

Just as I explained, the problem isn't a recursion bug, but just noise
in the trace.

Some of what is called by unwind_frame() will always be traced, and I
wouldn't want 'notrace' annotation on those.

If anything, I would just suggest another config option that lets the
user decide if they don't mind the messiness of the trace for the added
benefit of where the latency output came from.

Also, it's rather trivial to make the entire unwind.c not traced, by
adding in the Makefile:

CFLAGS_REMOVE_unwind.o = -pg

But that only stops the tracing of the unwind_*. Looks to be a lot of
those. But it wont stop the tracing of:

 kernel_text_address <-unwind_frame
 core_kernel_text <-unwind_frame
 search_index <-unwind_frame
 etc.

Heck, the user could just keep those from being traced by doing:

 echo kernel_text_address core_kernel_text search_index > \
   /sys/kernel/debug/tracing/set_ftrace_notrace

If DYNAMIC_FTRACE is defined.

-- Steve

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

[Keun-O Park]

On Fri, Jan 4, 2013 at 1:58 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2013-01-03 at 16:23 +0000, Russell King - ARM Linux wrote:
>> On Thu, Jan 03, 2013 at 11:03:58AM -0500, Steven Rostedt wrote:
>> > On Thu, 2013-01-03 at 14:13 +0000, Russell King - ARM Linux wrote:
>> > > In summary, from what I can see in the patch, the reason why the ifdefs
>> > > are the way they are, and the reason the warning is there has not been
>> > > addressed; these patches just seem to be aimed just at removing a #warning
>> > > statement to make the warning go away.
>> >
>> > You're correct that this patch does not solve any of theses issues. Now,
>> > I'm thinking that ftrace has matured where these issues don't exist, and
>> > where they do, it will only cause noise in the trace than anything
>> > serious. But, this needs to be looked deeper to make sure.
>>
>> Looking back in the archives, it seems that we had a problem with ftrace
>> and the unwinder polluting the trace information:
>>
>> http://lists.arm.linux.org.uk/lurker/message/20090604.201745.1c41ee6c.en.html
>>
>> There's quite a bit of discussion in that thread about this which details
>> why we came up with what we have today.
>
> Thanks for the link. In fact, I see I was even involved :-)
>
> http://lists.arm.linux.org.uk/lurker/message/20090609.213448.b4e4d096.en.html
>
> Just as I explained, the problem isn't a recursion bug, but just noise
> in the trace.
>
> Some of what is called by unwind_frame() will always be traced, and I
> wouldn't want 'notrace' annotation on those.
>
> If anything, I would just suggest another config option that lets the
> user decide if they don't mind the messiness of the trace for the added
> benefit of where the latency output came from.
>
> Also, it's rather trivial to make the entire unwind.c not traced, by
> adding in the Makefile:
>
> CFLAGS_REMOVE_unwind.o = -pg
>
> But that only stops the tracing of the unwind_*. Looks to be a lot of
> those. But it wont stop the tracing of:
>
>  kernel_text_address <-unwind_frame
>  core_kernel_text <-unwind_frame
>  search_index <-unwind_frame
>  etc.
>
> Heck, the user could just keep those from being traced by doing:
>
>  echo kernel_text_address core_kernel_text search_index > \
>    /sys/kernel/debug/tracing/set_ftrace_notrace
>
> If DYNAMIC_FTRACE is defined.
>
> -- Steve
>
>

With "CFLAGS_REMOVE_unwind.o = -pg" and with CONFIG_PROVE_LOCKING
turned on, I confirmed that
there's no trace output like Steve mentioned.
However, if I turn off CONFIG_PROVE_LOCKING, "irqsoff" and
"preemptirqsoff" ftracer prints these lines :

kernel_text_address <-unwind_frame
core_kernel_text <-unwind_frame
search_index <-unwind_frame

While I investigated the reason, I just found out there's two function
with same name, trace_hardirqs_off.
One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
And both symbols are exported.
I am wondering whether it is intentionally maintained code to
manipulate ftrace and lockdep or not.
I guess it's probably not.

-- Kpark

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

[Steven Rostedt]

On Fri, 2013-01-04 at 17:45 +0900, Keun-O Park wrote:

> With "CFLAGS_REMOVE_unwind.o = -pg" and with CONFIG_PROVE_LOCKING
> turned on, I confirmed that
> there's no trace output like Steve mentioned.
> However, if I turn off CONFIG_PROVE_LOCKING, "irqsoff" and
> "preemptirqsoff" ftracer prints these lines :
> 
> kernel_text_address <-unwind_frame
> core_kernel_text <-unwind_frame
> search_index <-unwind_frame
> 
> While I investigated the reason, I just found out there's two function
> with same name, trace_hardirqs_off.
> One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
> And both symbols are exported.
> I am wondering whether it is intentionally maintained code to
> manipulate ftrace and lockdep or not.
> I guess it's probably not.

Both the irqsoff tracer from ftrace and lockdep came from the -rt patch.
The two were very integrated at the time. When they were ported over to
mainline, they still used the same infrastructure to hook into the
locations of interrupts being disabled or enabled.

trace_hardirqs_on/off() is the function that is called for both lockdep
and ftrace's irqsoff tracer. So this was intentional. That way we didn't
need to have two different callers at every location. But if lockdep
isn't defined and ftrace irqsoff is, then ftrace would define the
trace_hardirqs_on/off() routines, otherwise lockdep does. (These
routines are within CONFIG_PROVE_LOCKING #ifdefs in
kernel/trace/trace_irqsoff.c)

When both lockdep and irqsoff tracer are configured, then lockdep
defines the trace_hardirqs_off_caller(), and calls time_hardirqs_off()
in trace_irqsoff.c which does the same thing as the trace_hardirqs_off()
does without lockdep.

I'm not sure why one would add the annotations while adding
PROVE_LOCKING doesn't. They both seems to use CALLER_ADDR0, but maybe
there's another path that uses CALLER_ADDR1 without it.

-- Steve

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

[Keun-O Park]

On Mon, Jan 7, 2013 at 10:41 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 2013-01-04 at 17:45 +0900, Keun-O Park wrote:
>
> > With "CFLAGS_REMOVE_unwind.o = -pg" and with CONFIG_PROVE_LOCKING
> > turned on, I confirmed that
> > there's no trace output like Steve mentioned.
> > However, if I turn off CONFIG_PROVE_LOCKING, "irqsoff" and
> > "preemptirqsoff" ftracer prints these lines :
> >
> > kernel_text_address <-unwind_frame
> > core_kernel_text <-unwind_frame
> > search_index <-unwind_frame
> >
> > While I investigated the reason, I just found out there's two function
> > with same name, trace_hardirqs_off.
> > One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c.
> > And both symbols are exported.
> > I am wondering whether it is intentionally maintained code to
> > manipulate ftrace and lockdep or not.
> > I guess it's probably not.
>
> Both the irqsoff tracer from ftrace and lockdep came from the -rt patch.
> The two were very integrated at the time. When they were ported over to
> mainline, they still used the same infrastructure to hook into the
> locations of interrupts being disabled or enabled.
>
> trace_hardirqs_on/off() is the function that is called for both lockdep
> and ftrace's irqsoff tracer. So this was intentional. That way we didn't
> need to have two different callers at every location. But if lockdep
> isn't defined and ftrace irqsoff is, then ftrace would define the
> trace_hardirqs_on/off() routines, otherwise lockdep does. (These
> routines are within CONFIG_PROVE_LOCKING #ifdefs in
> kernel/trace/trace_irqsoff.c)
>
> When both lockdep and irqsoff tracer are configured, then lockdep
> defines the trace_hardirqs_off_caller(), and calls time_hardirqs_off()
> in trace_irqsoff.c which does the same thing as the trace_hardirqs_off()
> does without lockdep.
>
> I'm not sure why one would add the annotations while adding
> PROVE_LOCKING doesn't. They both seems to use CALLER_ADDR0, but maybe
> there's another path that uses CALLER_ADDR1 without it.
>
> -- Steve
>
>

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.
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
 endif

 CFLAGS_REMOVE_return_address.o = -pg
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index fafedd8..f202bc0 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -11,7 +11,7 @@
 #include <linux/export.h>
 #include <linux/ftrace.h>

-#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
+#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
 #include <linux/sched.h>

 #include <asm/stacktrace.h>
@@ -57,17 +57,13 @@ void *return_address(unsigned int level)
 		return NULL;
 }

-#else /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) */
-
-#if defined(CONFIG_ARM_UNWIND)
-#warning "TODO: return_address should use unwind tables"
-#endif
+#else /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */

 void *return_address(unsigned int level)
 {
 	return NULL;
 }

-#endif /* if defined(CONFIG_FRAME_POINTER) &&
!defined(CONFIG_ARM_UNWIND) / else */
+#endif /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */

 EXPORT_SYMBOL_GPL(return_address);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 00f79e5..aab144b 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -6,6 +6,9 @@

 #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
 /*
+ * If both CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses
+ * unwind information. So for now just depend on !CONFIG_ARM_UNWIND.
+ *
  * Unwind the current stack frame and store the new register values in the
  * structure passed as argument. Unwinding is equivalent to a function return,
  * hence the new PC value rather than LR should be used for backtrace.
diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
index 713a2ca..6f207ed 100644
--- 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);
+
 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 */

Re: [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 */