| Message ID | 1357207949-3349-2-git-send-email-kpark3469@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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; >
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.
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
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.
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
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
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
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.