| Message ID | 1398851676-16389-2-git-send-email-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Akashi, On Wed, Apr 30, 2014 at 10:54:29AM +0100, AKASHI Takahiro wrote: > Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions > of CALLER_ADDRx(n), and so put them into linux/ftrace.h. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm/include/asm/ftrace.h | 10 +--------- > arch/blackfin/include/asm/ftrace.h | 11 +---------- > arch/parisc/include/asm/ftrace.h | 10 +--------- > arch/sh/include/asm/ftrace.h | 10 +--------- > arch/xtensa/include/asm/ftrace.h | 14 ++++---------- > include/linux/ftrace.h | 34 ++++++++++++++++++---------------- > 6 files changed, 26 insertions(+), 63 deletions(-) This one looks a bit too widespread to be merged via the arm64 tree. I wonder if the ftrace maintainers would consider taking it as a cleanup? Will > diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h > index f89515a..eb577f4 100644 > --- a/arch/arm/include/asm/ftrace.h > +++ b/arch/arm/include/asm/ftrace.h > @@ -52,15 +52,7 @@ extern inline void *return_address(unsigned int level) > > #endif > > -#define HAVE_ARCH_CALLER_ADDR > - > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 ((unsigned long)return_address(1)) > -#define CALLER_ADDR2 ((unsigned long)return_address(2)) > -#define CALLER_ADDR3 ((unsigned long)return_address(3)) > -#define CALLER_ADDR4 ((unsigned long)return_address(4)) > -#define CALLER_ADDR5 ((unsigned long)return_address(5)) > -#define CALLER_ADDR6 ((unsigned long)return_address(6)) > +#define ftrace_return_addr(n) return_address(n) > > #endif /* ifndef __ASSEMBLY__ */ > > diff --git a/arch/blackfin/include/asm/ftrace.h b/arch/blackfin/include/asm/ftrace.h > index 8a02950..2f1c3c2 100644 > --- a/arch/blackfin/include/asm/ftrace.h > +++ b/arch/blackfin/include/asm/ftrace.h > @@ -66,16 +66,7 @@ extern inline void *return_address(unsigned int level) > > #endif /* CONFIG_FRAME_POINTER */ > > -#define HAVE_ARCH_CALLER_ADDR > - > -/* inline function or macro may lead to unexpected result */ > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 ((unsigned long)return_address(1)) > -#define CALLER_ADDR2 ((unsigned long)return_address(2)) > -#define CALLER_ADDR3 ((unsigned long)return_address(3)) > -#define CALLER_ADDR4 ((unsigned long)return_address(4)) > -#define CALLER_ADDR5 ((unsigned long)return_address(5)) > -#define CALLER_ADDR6 ((unsigned long)return_address(6)) > +#define ftrace_return_address(n) return_address(n) > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/parisc/include/asm/ftrace.h b/arch/parisc/include/asm/ftrace.h > index 72c0faf..544ed8e 100644 > --- a/arch/parisc/include/asm/ftrace.h > +++ b/arch/parisc/include/asm/ftrace.h > @@ -24,15 +24,7 @@ extern void return_to_handler(void); > > extern unsigned long return_address(unsigned int); > > -#define HAVE_ARCH_CALLER_ADDR > - > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 return_address(1) > -#define CALLER_ADDR2 return_address(2) > -#define CALLER_ADDR3 return_address(3) > -#define CALLER_ADDR4 return_address(4) > -#define CALLER_ADDR5 return_address(5) > -#define CALLER_ADDR6 return_address(6) > +#define ftrace_return_address(n) return_address(n) > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/sh/include/asm/ftrace.h b/arch/sh/include/asm/ftrace.h > index 13e9966..e79fb6e 100644 > --- a/arch/sh/include/asm/ftrace.h > +++ b/arch/sh/include/asm/ftrace.h > @@ -40,15 +40,7 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) > /* arch/sh/kernel/return_address.c */ > extern void *return_address(unsigned int); > > -#define HAVE_ARCH_CALLER_ADDR > - > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 ((unsigned long)return_address(1)) > -#define CALLER_ADDR2 ((unsigned long)return_address(2)) > -#define CALLER_ADDR3 ((unsigned long)return_address(3)) > -#define CALLER_ADDR4 ((unsigned long)return_address(4)) > -#define CALLER_ADDR5 ((unsigned long)return_address(5)) > -#define CALLER_ADDR6 ((unsigned long)return_address(6)) > +#define ftrace_return_address(n) return_address(n) > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/xtensa/include/asm/ftrace.h b/arch/xtensa/include/asm/ftrace.h > index 736b9d2..6c6d9a9 100644 > --- a/arch/xtensa/include/asm/ftrace.h > +++ b/arch/xtensa/include/asm/ftrace.h > @@ -12,24 +12,18 @@ > > #include <asm/processor.h> > > -#define HAVE_ARCH_CALLER_ADDR > #ifndef __ASSEMBLY__ > -#define CALLER_ADDR0 ({ unsigned long a0, a1; \ > +#define ftrace_return_address0 ({ unsigned long a0, a1; \ > __asm__ __volatile__ ( \ > "mov %0, a0\n" \ > "mov %1, a1\n" \ > : "=r"(a0), "=r"(a1)); \ > MAKE_PC_FROM_RA(a0, a1); }) > + > #ifdef CONFIG_FRAME_POINTER > extern unsigned long return_address(unsigned level); > -#define CALLER_ADDR1 return_address(1) > -#define CALLER_ADDR2 return_address(2) > -#define CALLER_ADDR3 return_address(3) > -#else /* CONFIG_FRAME_POINTER */ > -#define CALLER_ADDR1 (0) > -#define CALLER_ADDR2 (0) > -#define CALLER_ADDR3 (0) > -#endif /* CONFIG_FRAME_POINTER */ > +#define ftrace_return_address(n) return_address(n) > +#endif > #endif /* __ASSEMBLY__ */ > > #ifdef CONFIG_FUNCTION_TRACER > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 9212b01..18f1ae7 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -614,25 +614,27 @@ static inline void __ftrace_enabled_restore(int enabled) > #endif > } > > -#ifndef HAVE_ARCH_CALLER_ADDR > +/* All archs should have this, but we define it for consistency */ > +#ifndef ftrace_return_address0 > +# define ftrace_return_address0 __builtin_return_address(0) > +#endif > + > +/* Archs may use other ways for ADDR1 and beyond */ > +#ifndef ftrace_return_address > # ifdef CONFIG_FRAME_POINTER > -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -# define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1)) > -# define CALLER_ADDR2 ((unsigned long)__builtin_return_address(2)) > -# define CALLER_ADDR3 ((unsigned long)__builtin_return_address(3)) > -# define CALLER_ADDR4 ((unsigned long)__builtin_return_address(4)) > -# define CALLER_ADDR5 ((unsigned long)__builtin_return_address(5)) > -# define CALLER_ADDR6 ((unsigned long)__builtin_return_address(6)) > +# define ftrace_return_address(n) __builtin_return_address(n) > # else > -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -# define CALLER_ADDR1 0UL > -# define CALLER_ADDR2 0UL > -# define CALLER_ADDR3 0UL > -# define CALLER_ADDR4 0UL > -# define CALLER_ADDR5 0UL > -# define CALLER_ADDR6 0UL > +# define ftrace_return_address(n) 0UL > # endif > -#endif /* ifndef HAVE_ARCH_CALLER_ADDR */ > +#endif > + > +#define CALLER_ADDR0 ((unsigned long)ftrace_return_address0) > +#define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1)) > +#define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2)) > +#define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3)) > +#define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4)) > +#define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5)) > +#define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6)) > > #ifdef CONFIG_IRQSOFF_TRACER > extern void time_hardirqs_on(unsigned long a0, unsigned long a1); > -- > 1.7.9.5 > >
On Fri, 2 May 2014 19:13:48 +0100 Will Deacon <will.deacon@arm.com> wrote: > Hi Akashi, > > On Wed, Apr 30, 2014 at 10:54:29AM +0100, AKASHI Takahiro wrote: > > Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions > > of CALLER_ADDRx(n), and so put them into linux/ftrace.h. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > arch/arm/include/asm/ftrace.h | 10 +--------- > > arch/blackfin/include/asm/ftrace.h | 11 +---------- > > arch/parisc/include/asm/ftrace.h | 10 +--------- > > arch/sh/include/asm/ftrace.h | 10 +--------- > > arch/xtensa/include/asm/ftrace.h | 14 ++++---------- > > include/linux/ftrace.h | 34 ++++++++++++++++++---------------- > > 6 files changed, 26 insertions(+), 63 deletions(-) > > This one looks a bit too widespread to be merged via the arm64 tree. I > wonder if the ftrace maintainers would consider taking it as a cleanup? > I actually was the one to recommend this approach. But I have some small comments to the patch. I'll reply directly to the patch with them. -- Steve
On Fri, 2 May 2014 19:13:48 +0100 Will Deacon <will.deacon@arm.com> wrote: > This one looks a bit too widespread to be merged via the arm64 tree. I > wonder if the ftrace maintainers would consider taking it as a cleanup? Oh, and I can take the patch if you feel uncomfortable with taking something so spread out. As it deals with ftrace I probably can get away with a cross the board arch change to it. -- Steve
On Wed, 30 Apr 2014 18:54:29 +0900 AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions > of CALLER_ADDRx(n), and so put them into linux/ftrace.h. Please add a bit more to the change log. Like what you did. Something like: ---- Most archs with HAVE_ARCH_CALLER_ADDR have pretty much the same definitions of CALLER_ADDRx(n). Instead of duplicating the code for all the archs, define a ftrace_return_address0() and ftrace_return_address(n) that can be overwritten by the archs if they need to do something different. Instead of 7 macros in every arch, we now only have at most 2 (and actually only 1 as ftrace_return_address0() should be the same for all archs). The CALLER_ADDRx(n) will now be defined in linux/ftrace.h and use the ftrace_return_address*(n?) macros. This removes a lot of the duplicate code. ----- Use that if you want :-) > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm/include/asm/ftrace.h | 10 +--------- > arch/blackfin/include/asm/ftrace.h | 11 +---------- > arch/parisc/include/asm/ftrace.h | 10 +--------- > arch/sh/include/asm/ftrace.h | 10 +--------- > arch/xtensa/include/asm/ftrace.h | 14 ++++---------- > include/linux/ftrace.h | 34 ++++++++++++++++++---------------- > 6 files changed, 26 insertions(+), 63 deletions(-) > > diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h > index f89515a..eb577f4 100644 > --- a/arch/arm/include/asm/ftrace.h > +++ b/arch/arm/include/asm/ftrace.h > @@ -52,15 +52,7 @@ extern inline void *return_address(unsigned int level) > > #endif > > -#define HAVE_ARCH_CALLER_ADDR > - > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 ((unsigned long)return_address(1)) > -#define CALLER_ADDR2 ((unsigned long)return_address(2)) > -#define CALLER_ADDR3 ((unsigned long)return_address(3)) > -#define CALLER_ADDR4 ((unsigned long)return_address(4)) > -#define CALLER_ADDR5 ((unsigned long)return_address(5)) > -#define CALLER_ADDR6 ((unsigned long)return_address(6)) > +#define ftrace_return_addr(n) return_address(n) > > #endif /* ifndef __ASSEMBLY__ */ > > diff --git a/arch/blackfin/include/asm/ftrace.h b/arch/blackfin/include/asm/ftrace.h > index 8a02950..2f1c3c2 100644 > --- a/arch/blackfin/include/asm/ftrace.h > +++ b/arch/blackfin/include/asm/ftrace.h > @@ -66,16 +66,7 @@ extern inline void *return_address(unsigned int level) > > #endif /* CONFIG_FRAME_POINTER */ > > -#define HAVE_ARCH_CALLER_ADDR > - > -/* inline function or macro may lead to unexpected result */ > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 ((unsigned long)return_address(1)) > -#define CALLER_ADDR2 ((unsigned long)return_address(2)) > -#define CALLER_ADDR3 ((unsigned long)return_address(3)) > -#define CALLER_ADDR4 ((unsigned long)return_address(4)) > -#define CALLER_ADDR5 ((unsigned long)return_address(5)) > -#define CALLER_ADDR6 ((unsigned long)return_address(6)) > +#define ftrace_return_address(n) return_address(n) > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/parisc/include/asm/ftrace.h b/arch/parisc/include/asm/ftrace.h > index 72c0faf..544ed8e 100644 > --- a/arch/parisc/include/asm/ftrace.h > +++ b/arch/parisc/include/asm/ftrace.h > @@ -24,15 +24,7 @@ extern void return_to_handler(void); > > extern unsigned long return_address(unsigned int); > > -#define HAVE_ARCH_CALLER_ADDR > - > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 return_address(1) > -#define CALLER_ADDR2 return_address(2) > -#define CALLER_ADDR3 return_address(3) > -#define CALLER_ADDR4 return_address(4) > -#define CALLER_ADDR5 return_address(5) > -#define CALLER_ADDR6 return_address(6) > +#define ftrace_return_address(n) return_address(n) > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/sh/include/asm/ftrace.h b/arch/sh/include/asm/ftrace.h > index 13e9966..e79fb6e 100644 > --- a/arch/sh/include/asm/ftrace.h > +++ b/arch/sh/include/asm/ftrace.h > @@ -40,15 +40,7 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) > /* arch/sh/kernel/return_address.c */ > extern void *return_address(unsigned int); > > -#define HAVE_ARCH_CALLER_ADDR > - > -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -#define CALLER_ADDR1 ((unsigned long)return_address(1)) > -#define CALLER_ADDR2 ((unsigned long)return_address(2)) > -#define CALLER_ADDR3 ((unsigned long)return_address(3)) > -#define CALLER_ADDR4 ((unsigned long)return_address(4)) > -#define CALLER_ADDR5 ((unsigned long)return_address(5)) > -#define CALLER_ADDR6 ((unsigned long)return_address(6)) > +#define ftrace_return_address(n) return_address(n) > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/xtensa/include/asm/ftrace.h b/arch/xtensa/include/asm/ftrace.h > index 736b9d2..6c6d9a9 100644 > --- a/arch/xtensa/include/asm/ftrace.h > +++ b/arch/xtensa/include/asm/ftrace.h > @@ -12,24 +12,18 @@ > > #include <asm/processor.h> > > -#define HAVE_ARCH_CALLER_ADDR > #ifndef __ASSEMBLY__ > -#define CALLER_ADDR0 ({ unsigned long a0, a1; \ > +#define ftrace_return_address0 ({ unsigned long a0, a1; \ > __asm__ __volatile__ ( \ > "mov %0, a0\n" \ > "mov %1, a1\n" \ > : "=r"(a0), "=r"(a1)); \ > MAKE_PC_FROM_RA(a0, a1); }) > + > #ifdef CONFIG_FRAME_POINTER > extern unsigned long return_address(unsigned level); > -#define CALLER_ADDR1 return_address(1) > -#define CALLER_ADDR2 return_address(2) > -#define CALLER_ADDR3 return_address(3) > -#else /* CONFIG_FRAME_POINTER */ > -#define CALLER_ADDR1 (0) > -#define CALLER_ADDR2 (0) > -#define CALLER_ADDR3 (0) > -#endif /* CONFIG_FRAME_POINTER */ > +#define ftrace_return_address(n) return_address(n) I would add a comment here that states: /* * #else !CONFIG_FRAME_POINTER * * Define CALLER_ADDRn (n > 0) to 0 is the default in linux/ftrace.h if * ftrace_return_address(n) and CONFIG_FRAME_POINTER are not defined. */ Otherwise it looks like you are missing the #else statement. > +#endif > #endif /* __ASSEMBLY__ */ > > #ifdef CONFIG_FUNCTION_TRACER > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 9212b01..18f1ae7 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -614,25 +614,27 @@ static inline void __ftrace_enabled_restore(int enabled) > #endif > } > > -#ifndef HAVE_ARCH_CALLER_ADDR > +/* All archs should have this, but we define it for consistency */ The comment is a little confusing. I think it may be better stated as: /* * All archs should be able to use __builtin_return_address(0) but * we allow them to redefine it for consistency. */ > +#ifndef ftrace_return_address0 > +# define ftrace_return_address0 __builtin_return_address(0) > +#endif > + > +/* Archs may use other ways for ADDR1 and beyond */ How about: /* Not all archs can use __builtin_return_address(n) where n > 0 */ > +#ifndef ftrace_return_address > # ifdef CONFIG_FRAME_POINTER > -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -# define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1)) > -# define CALLER_ADDR2 ((unsigned long)__builtin_return_address(2)) > -# define CALLER_ADDR3 ((unsigned long)__builtin_return_address(3)) > -# define CALLER_ADDR4 ((unsigned long)__builtin_return_address(4)) > -# define CALLER_ADDR5 ((unsigned long)__builtin_return_address(5)) > -# define CALLER_ADDR6 ((unsigned long)__builtin_return_address(6)) > +# define ftrace_return_address(n) __builtin_return_address(n) > # else > -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > -# define CALLER_ADDR1 0UL > -# define CALLER_ADDR2 0UL > -# define CALLER_ADDR3 0UL > -# define CALLER_ADDR4 0UL > -# define CALLER_ADDR5 0UL > -# define CALLER_ADDR6 0UL > +# define ftrace_return_address(n) 0UL > # endif > -#endif /* ifndef HAVE_ARCH_CALLER_ADDR */ > +#endif > + > +#define CALLER_ADDR0 ((unsigned long)ftrace_return_address0) > +#define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1)) > +#define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2)) > +#define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3)) > +#define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4)) > +#define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5)) > +#define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6)) Other than that, it looks good. You can send me the patch and I'll add it to my 3.16 queue. Feel free to reply to this email with a v9. When I pull patches into git, it adds the link to the thread for the patch in LKML. To keep this entire thread, just reply to it. Thanks! -- Steve > > #ifdef CONFIG_IRQSOFF_TRACER > extern void time_hardirqs_on(unsigned long a0, unsigned long a1);
Hi guys, On Fri, May 02, 2014 at 08:19:57PM +0100, Steven Rostedt wrote: > On Wed, 30 Apr 2014 18:54:29 +0900 > AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions > > of CALLER_ADDRx(n), and so put them into linux/ftrace.h. > > Please add a bit more to the change log. Like what you did. Something > like: > > ---- > Most archs with HAVE_ARCH_CALLER_ADDR have pretty much the same > definitions of CALLER_ADDRx(n). Instead of duplicating the code for all > the archs, define a ftrace_return_address0() and > ftrace_return_address(n) that can be overwritten by the archs if they > need to do something different. Instead of 7 macros in every arch, we > now only have at most 2 (and actually only 1 as > ftrace_return_address0() should be the same for all archs). > > The CALLER_ADDRx(n) will now be defined in linux/ftrace.h and use the > ftrace_return_address*(n?) macros. This removes a lot of the duplicate > code. > ----- > > Use that if you want :-) Akashi: did you get around to posting a new version of this patch? We can't take your arm64 patches until you get the core stuff merged... Will
On Mon, 12 May 2014 16:58:11 +0100 Will Deacon <will.deacon@arm.com> wrote: > Akashi: did you get around to posting a new version of this patch? We can't > take your arm64 patches until you get the core stuff merged... I haven't seen any patches yet. What I can also do is to create a separate branch based on mainline, and just apply this change to the core. Then you could pull that branch with a note to Linus that it is also in my tree with the common ancestor. If he pulls your work first, he'll only get that change from my tree, and if he pulls mine first it doesn't matter as all the changes will be there. This is something he said at Kernel Summit that was normal procedure if there's a change in one tree that another tree is dependent on. -- Steve
Hey Steve, On Mon, May 12, 2014 at 05:05:35PM +0100, Steven Rostedt wrote: > On Mon, 12 May 2014 16:58:11 +0100 > Will Deacon <will.deacon@arm.com> wrote: > > > Akashi: did you get around to posting a new version of this patch? We can't > > take your arm64 patches until you get the core stuff merged... > > I haven't seen any patches yet. > > What I can also do is to create a separate branch based on mainline, > and just apply this change to the core. Then you could pull that branch > with a note to Linus that it is also in my tree with the common > ancestor. > > If he pulls your work first, he'll only get that change from my tree, > and if he pulls mine first it doesn't matter as all the changes will be > there. > > This is something he said at Kernel Summit that was normal procedure if > there's a change in one tree that another tree is dependent on. That sounds great, and nicely solves the dependency between our trees for 3.16 at the same time. If you point us at the branch and promise not to rebase it, that would be fantastic. Cheers, Will
On Mon, 12 May 2014 17:12:46 +0100 Will Deacon <will.deacon@arm.com> wrote: > If you point us at the branch and promise not to rebase it, that would be > fantastic. I will when I get the patch(es) :-) and run it through all my tests. -- Steve
Hi Will, Steven, So sorry, I completely missed this message thread. I will submit a new patch (replacement of [1/8]) in the following e-mail. -Takahiro AKASHI On 05/03/2014 04:19 AM, Steven Rostedt wrote: > On Wed, 30 Apr 2014 18:54:29 +0900 > AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > >> Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions >> of CALLER_ADDRx(n), and so put them into linux/ftrace.h. > > Please add a bit more to the change log. Like what you did. Something > like: > > ---- > Most archs with HAVE_ARCH_CALLER_ADDR have pretty much the same > definitions of CALLER_ADDRx(n). Instead of duplicating the code for all > the archs, define a ftrace_return_address0() and > ftrace_return_address(n) that can be overwritten by the archs if they > need to do something different. Instead of 7 macros in every arch, we > now only have at most 2 (and actually only 1 as > ftrace_return_address0() should be the same for all archs). > > The CALLER_ADDRx(n) will now be defined in linux/ftrace.h and use the > ftrace_return_address*(n?) macros. This removes a lot of the duplicate > code. > ----- > > Use that if you want :-) > > >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> arch/arm/include/asm/ftrace.h | 10 +--------- >> arch/blackfin/include/asm/ftrace.h | 11 +---------- >> arch/parisc/include/asm/ftrace.h | 10 +--------- >> arch/sh/include/asm/ftrace.h | 10 +--------- >> arch/xtensa/include/asm/ftrace.h | 14 ++++---------- >> include/linux/ftrace.h | 34 ++++++++++++++++++---------------- >> 6 files changed, 26 insertions(+), 63 deletions(-) >> >> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h >> index f89515a..eb577f4 100644 >> --- a/arch/arm/include/asm/ftrace.h >> +++ b/arch/arm/include/asm/ftrace.h >> @@ -52,15 +52,7 @@ extern inline void *return_address(unsigned int level) >> >> #endif >> >> -#define HAVE_ARCH_CALLER_ADDR >> - >> -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> -#define CALLER_ADDR1 ((unsigned long)return_address(1)) >> -#define CALLER_ADDR2 ((unsigned long)return_address(2)) >> -#define CALLER_ADDR3 ((unsigned long)return_address(3)) >> -#define CALLER_ADDR4 ((unsigned long)return_address(4)) >> -#define CALLER_ADDR5 ((unsigned long)return_address(5)) >> -#define CALLER_ADDR6 ((unsigned long)return_address(6)) >> +#define ftrace_return_addr(n) return_address(n) >> >> #endif /* ifndef __ASSEMBLY__ */ >> >> diff --git a/arch/blackfin/include/asm/ftrace.h b/arch/blackfin/include/asm/ftrace.h >> index 8a02950..2f1c3c2 100644 >> --- a/arch/blackfin/include/asm/ftrace.h >> +++ b/arch/blackfin/include/asm/ftrace.h >> @@ -66,16 +66,7 @@ extern inline void *return_address(unsigned int level) >> >> #endif /* CONFIG_FRAME_POINTER */ >> >> -#define HAVE_ARCH_CALLER_ADDR >> - >> -/* inline function or macro may lead to unexpected result */ >> -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> -#define CALLER_ADDR1 ((unsigned long)return_address(1)) >> -#define CALLER_ADDR2 ((unsigned long)return_address(2)) >> -#define CALLER_ADDR3 ((unsigned long)return_address(3)) >> -#define CALLER_ADDR4 ((unsigned long)return_address(4)) >> -#define CALLER_ADDR5 ((unsigned long)return_address(5)) >> -#define CALLER_ADDR6 ((unsigned long)return_address(6)) >> +#define ftrace_return_address(n) return_address(n) >> >> #endif /* __ASSEMBLY__ */ >> >> diff --git a/arch/parisc/include/asm/ftrace.h b/arch/parisc/include/asm/ftrace.h >> index 72c0faf..544ed8e 100644 >> --- a/arch/parisc/include/asm/ftrace.h >> +++ b/arch/parisc/include/asm/ftrace.h >> @@ -24,15 +24,7 @@ extern void return_to_handler(void); >> >> extern unsigned long return_address(unsigned int); >> >> -#define HAVE_ARCH_CALLER_ADDR >> - >> -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> -#define CALLER_ADDR1 return_address(1) >> -#define CALLER_ADDR2 return_address(2) >> -#define CALLER_ADDR3 return_address(3) >> -#define CALLER_ADDR4 return_address(4) >> -#define CALLER_ADDR5 return_address(5) >> -#define CALLER_ADDR6 return_address(6) >> +#define ftrace_return_address(n) return_address(n) >> >> #endif /* __ASSEMBLY__ */ >> >> diff --git a/arch/sh/include/asm/ftrace.h b/arch/sh/include/asm/ftrace.h >> index 13e9966..e79fb6e 100644 >> --- a/arch/sh/include/asm/ftrace.h >> +++ b/arch/sh/include/asm/ftrace.h >> @@ -40,15 +40,7 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) >> /* arch/sh/kernel/return_address.c */ >> extern void *return_address(unsigned int); >> >> -#define HAVE_ARCH_CALLER_ADDR >> - >> -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> -#define CALLER_ADDR1 ((unsigned long)return_address(1)) >> -#define CALLER_ADDR2 ((unsigned long)return_address(2)) >> -#define CALLER_ADDR3 ((unsigned long)return_address(3)) >> -#define CALLER_ADDR4 ((unsigned long)return_address(4)) >> -#define CALLER_ADDR5 ((unsigned long)return_address(5)) >> -#define CALLER_ADDR6 ((unsigned long)return_address(6)) >> +#define ftrace_return_address(n) return_address(n) >> >> #endif /* __ASSEMBLY__ */ >> >> diff --git a/arch/xtensa/include/asm/ftrace.h b/arch/xtensa/include/asm/ftrace.h >> index 736b9d2..6c6d9a9 100644 >> --- a/arch/xtensa/include/asm/ftrace.h >> +++ b/arch/xtensa/include/asm/ftrace.h >> @@ -12,24 +12,18 @@ >> >> #include <asm/processor.h> >> >> -#define HAVE_ARCH_CALLER_ADDR >> #ifndef __ASSEMBLY__ >> -#define CALLER_ADDR0 ({ unsigned long a0, a1; \ >> +#define ftrace_return_address0 ({ unsigned long a0, a1; \ >> __asm__ __volatile__ ( \ >> "mov %0, a0\n" \ >> "mov %1, a1\n" \ >> : "=r"(a0), "=r"(a1)); \ >> MAKE_PC_FROM_RA(a0, a1); }) >> + >> #ifdef CONFIG_FRAME_POINTER >> extern unsigned long return_address(unsigned level); >> -#define CALLER_ADDR1 return_address(1) >> -#define CALLER_ADDR2 return_address(2) >> -#define CALLER_ADDR3 return_address(3) >> -#else /* CONFIG_FRAME_POINTER */ >> -#define CALLER_ADDR1 (0) >> -#define CALLER_ADDR2 (0) >> -#define CALLER_ADDR3 (0) >> -#endif /* CONFIG_FRAME_POINTER */ >> +#define ftrace_return_address(n) return_address(n) > > I would add a comment here that states: > > /* > * #else !CONFIG_FRAME_POINTER > * > * Define CALLER_ADDRn (n > 0) to 0 is the default in linux/ftrace.h if > * ftrace_return_address(n) and CONFIG_FRAME_POINTER are not defined. > */ > > Otherwise it looks like you are missing the #else statement. > >> +#endif >> #endif /* __ASSEMBLY__ */ >> >> #ifdef CONFIG_FUNCTION_TRACER >> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h >> index 9212b01..18f1ae7 100644 >> --- a/include/linux/ftrace.h >> +++ b/include/linux/ftrace.h >> @@ -614,25 +614,27 @@ static inline void __ftrace_enabled_restore(int enabled) >> #endif >> } >> >> -#ifndef HAVE_ARCH_CALLER_ADDR >> +/* All archs should have this, but we define it for consistency */ > > The comment is a little confusing. I think it may be better stated as: > > /* > * All archs should be able to use __builtin_return_address(0) but > * we allow them to redefine it for consistency. > */ > >> +#ifndef ftrace_return_address0 >> +# define ftrace_return_address0 __builtin_return_address(0) >> +#endif >> + >> +/* Archs may use other ways for ADDR1 and beyond */ > > How about: > > /* Not all archs can use __builtin_return_address(n) where n > 0 */ > >> +#ifndef ftrace_return_address >> # ifdef CONFIG_FRAME_POINTER >> -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> -# define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1)) >> -# define CALLER_ADDR2 ((unsigned long)__builtin_return_address(2)) >> -# define CALLER_ADDR3 ((unsigned long)__builtin_return_address(3)) >> -# define CALLER_ADDR4 ((unsigned long)__builtin_return_address(4)) >> -# define CALLER_ADDR5 ((unsigned long)__builtin_return_address(5)) >> -# define CALLER_ADDR6 ((unsigned long)__builtin_return_address(6)) >> +# define ftrace_return_address(n) __builtin_return_address(n) >> # else >> -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> -# define CALLER_ADDR1 0UL >> -# define CALLER_ADDR2 0UL >> -# define CALLER_ADDR3 0UL >> -# define CALLER_ADDR4 0UL >> -# define CALLER_ADDR5 0UL >> -# define CALLER_ADDR6 0UL >> +# define ftrace_return_address(n) 0UL >> # endif >> -#endif /* ifndef HAVE_ARCH_CALLER_ADDR */ >> +#endif >> + >> +#define CALLER_ADDR0 ((unsigned long)ftrace_return_address0) >> +#define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1)) >> +#define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2)) >> +#define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3)) >> +#define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4)) >> +#define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5)) >> +#define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6)) > > Other than that, it looks good. You can send me the patch and I'll add > it to my 3.16 queue. > > Feel free to reply to this email with a v9. When I pull patches into > git, it adds the link to the thread for the patch in LKML. To keep this > entire thread, just reply to it. > > Thanks! > > -- Steve > >> >> #ifdef CONFIG_IRQSOFF_TRACER >> extern void time_hardirqs_on(unsigned long a0, unsigned long a1); >
On Wed, Apr 30, 2014 at 11:54 AM, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions > of CALLER_ADDRx(n), and so put them into linux/ftrace.h. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> On arm (at least shmobile_defconfig and versatile_defconfig) with gcc 4.6.3: kernel/sched/core.c: In function 'get_parent_ip': kernel/sched/core.c:2520:10: warning: unsupported argument to '__builtin_return_address' [enabled by default] kernel/sched/core.c:2522:11: warning: unsupported argument to '__builtin_return_address' [enabled by default] With gcc 4.9.0: kernel/sched/core.c: In function 'get_parent_ip': include/linux/ftrace.h:627:36: warning: unsupported argument to '__builtin_return_address' # define ftrace_return_address(n) __builtin_return_address(n) ^ include/linux/ftrace.h:635:38: note: in expansion of macro 'ftrace_return_address' #define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2)) ^ kernel/sched/core.c:2520:10: note: in expansion of macro 'CALLER_ADDR2' addr = CALLER_ADDR2; ^ include/linux/ftrace.h:627:36: warning: unsupported argument to '__builtin_return_address' # define ftrace_return_address(n) __builtin_return_address(n) ^ include/linux/ftrace.h:636:38: note: in expansion of macro 'ftrace_return_address' #define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3)) ^ kernel/sched/core.c:2522:11: note: in expansion of macro 'CALLER_ADDR3' addr = CALLER_ADDR3; ^ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h index f89515a..eb577f4 100644 --- a/arch/arm/include/asm/ftrace.h +++ b/arch/arm/include/asm/ftrace.h @@ -52,15 +52,7 @@ extern inline void *return_address(unsigned int level) #endif -#define HAVE_ARCH_CALLER_ADDR - -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) -#define CALLER_ADDR1 ((unsigned long)return_address(1)) -#define CALLER_ADDR2 ((unsigned long)return_address(2)) -#define CALLER_ADDR3 ((unsigned long)return_address(3)) -#define CALLER_ADDR4 ((unsigned long)return_address(4)) -#define CALLER_ADDR5 ((unsigned long)return_address(5)) -#define CALLER_ADDR6 ((unsigned long)return_address(6)) +#define ftrace_return_addr(n) return_address(n) #endif /* ifndef __ASSEMBLY__ */ diff --git a/arch/blackfin/include/asm/ftrace.h b/arch/blackfin/include/asm/ftrace.h index 8a02950..2f1c3c2 100644 --- a/arch/blackfin/include/asm/ftrace.h +++ b/arch/blackfin/include/asm/ftrace.h @@ -66,16 +66,7 @@ extern inline void *return_address(unsigned int level) #endif /* CONFIG_FRAME_POINTER */ -#define HAVE_ARCH_CALLER_ADDR - -/* inline function or macro may lead to unexpected result */ -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) -#define CALLER_ADDR1 ((unsigned long)return_address(1)) -#define CALLER_ADDR2 ((unsigned long)return_address(2)) -#define CALLER_ADDR3 ((unsigned long)return_address(3)) -#define CALLER_ADDR4 ((unsigned long)return_address(4)) -#define CALLER_ADDR5 ((unsigned long)return_address(5)) -#define CALLER_ADDR6 ((unsigned long)return_address(6)) +#define ftrace_return_address(n) return_address(n) #endif /* __ASSEMBLY__ */ diff --git a/arch/parisc/include/asm/ftrace.h b/arch/parisc/include/asm/ftrace.h index 72c0faf..544ed8e 100644 --- a/arch/parisc/include/asm/ftrace.h +++ b/arch/parisc/include/asm/ftrace.h @@ -24,15 +24,7 @@ extern void return_to_handler(void); extern unsigned long return_address(unsigned int); -#define HAVE_ARCH_CALLER_ADDR - -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) -#define CALLER_ADDR1 return_address(1) -#define CALLER_ADDR2 return_address(2) -#define CALLER_ADDR3 return_address(3) -#define CALLER_ADDR4 return_address(4) -#define CALLER_ADDR5 return_address(5) -#define CALLER_ADDR6 return_address(6) +#define ftrace_return_address(n) return_address(n) #endif /* __ASSEMBLY__ */ diff --git a/arch/sh/include/asm/ftrace.h b/arch/sh/include/asm/ftrace.h index 13e9966..e79fb6e 100644 --- a/arch/sh/include/asm/ftrace.h +++ b/arch/sh/include/asm/ftrace.h @@ -40,15 +40,7 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) /* arch/sh/kernel/return_address.c */ extern void *return_address(unsigned int); -#define HAVE_ARCH_CALLER_ADDR - -#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) -#define CALLER_ADDR1 ((unsigned long)return_address(1)) -#define CALLER_ADDR2 ((unsigned long)return_address(2)) -#define CALLER_ADDR3 ((unsigned long)return_address(3)) -#define CALLER_ADDR4 ((unsigned long)return_address(4)) -#define CALLER_ADDR5 ((unsigned long)return_address(5)) -#define CALLER_ADDR6 ((unsigned long)return_address(6)) +#define ftrace_return_address(n) return_address(n) #endif /* __ASSEMBLY__ */ diff --git a/arch/xtensa/include/asm/ftrace.h b/arch/xtensa/include/asm/ftrace.h index 736b9d2..6c6d9a9 100644 --- a/arch/xtensa/include/asm/ftrace.h +++ b/arch/xtensa/include/asm/ftrace.h @@ -12,24 +12,18 @@ #include <asm/processor.h> -#define HAVE_ARCH_CALLER_ADDR #ifndef __ASSEMBLY__ -#define CALLER_ADDR0 ({ unsigned long a0, a1; \ +#define ftrace_return_address0 ({ unsigned long a0, a1; \ __asm__ __volatile__ ( \ "mov %0, a0\n" \ "mov %1, a1\n" \ : "=r"(a0), "=r"(a1)); \ MAKE_PC_FROM_RA(a0, a1); }) + #ifdef CONFIG_FRAME_POINTER extern unsigned long return_address(unsigned level); -#define CALLER_ADDR1 return_address(1) -#define CALLER_ADDR2 return_address(2) -#define CALLER_ADDR3 return_address(3) -#else /* CONFIG_FRAME_POINTER */ -#define CALLER_ADDR1 (0) -#define CALLER_ADDR2 (0) -#define CALLER_ADDR3 (0) -#endif /* CONFIG_FRAME_POINTER */ +#define ftrace_return_address(n) return_address(n) +#endif #endif /* __ASSEMBLY__ */ #ifdef CONFIG_FUNCTION_TRACER diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 9212b01..18f1ae7 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -614,25 +614,27 @@ static inline void __ftrace_enabled_restore(int enabled) #endif } -#ifndef HAVE_ARCH_CALLER_ADDR +/* All archs should have this, but we define it for consistency */ +#ifndef ftrace_return_address0 +# define ftrace_return_address0 __builtin_return_address(0) +#endif + +/* Archs may use other ways for ADDR1 and beyond */ +#ifndef ftrace_return_address # ifdef CONFIG_FRAME_POINTER -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) -# define CALLER_ADDR1 ((unsigned long)__builtin_return_address(1)) -# define CALLER_ADDR2 ((unsigned long)__builtin_return_address(2)) -# define CALLER_ADDR3 ((unsigned long)__builtin_return_address(3)) -# define CALLER_ADDR4 ((unsigned long)__builtin_return_address(4)) -# define CALLER_ADDR5 ((unsigned long)__builtin_return_address(5)) -# define CALLER_ADDR6 ((unsigned long)__builtin_return_address(6)) +# define ftrace_return_address(n) __builtin_return_address(n) # else -# define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) -# define CALLER_ADDR1 0UL -# define CALLER_ADDR2 0UL -# define CALLER_ADDR3 0UL -# define CALLER_ADDR4 0UL -# define CALLER_ADDR5 0UL -# define CALLER_ADDR6 0UL +# define ftrace_return_address(n) 0UL # endif -#endif /* ifndef HAVE_ARCH_CALLER_ADDR */ +#endif + +#define CALLER_ADDR0 ((unsigned long)ftrace_return_address0) +#define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1)) +#define CALLER_ADDR2 ((unsigned long)ftrace_return_address(2)) +#define CALLER_ADDR3 ((unsigned long)ftrace_return_address(3)) +#define CALLER_ADDR4 ((unsigned long)ftrace_return_address(4)) +#define CALLER_ADDR5 ((unsigned long)ftrace_return_address(5)) +#define CALLER_ADDR6 ((unsigned long)ftrace_return_address(6)) #ifdef CONFIG_IRQSOFF_TRACER extern void time_hardirqs_on(unsigned long a0, unsigned long a1);
Most archs with HAVE_ARCH_CALLER_ADDR have the almost same definitions of CALLER_ADDRx(n), and so put them into linux/ftrace.h. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm/include/asm/ftrace.h | 10 +--------- arch/blackfin/include/asm/ftrace.h | 11 +---------- arch/parisc/include/asm/ftrace.h | 10 +--------- arch/sh/include/asm/ftrace.h | 10 +--------- arch/xtensa/include/asm/ftrace.h | 14 ++++---------- include/linux/ftrace.h | 34 ++++++++++++++++++---------------- 6 files changed, 26 insertions(+), 63 deletions(-)