Message ID | 1459480972-20238-1-git-send-email-bauerman@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Hi Thiago, [auto build test ERROR on v4.6-rc1] [also build test ERROR on next-20160401] [cannot apply to tip/perf/core] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Thiago-Jung-Bauermann/ftrace-filter-Match-dot-symbols-when-searching-functions-on-ppc64/20160401-112617 config: powerpc-ppc6xx_defconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones prefixed by >>): In file included from include/linux/ftrace.h:20:0, from include/linux/perf_event.h:47, from include/linux/trace_events.h:9, from include/trace/syscall.h:6, from include/linux/syscalls.h:81, from arch/powerpc/kernel/pci-common.c:29: >> arch/powerpc/include/asm/ftrace.h:62:5: error: "CONFIG_PPC64" is not defined [-Werror=undef] #if CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) ^ cc1: all warnings being treated as errors vim +/CONFIG_PPC64 +62 arch/powerpc/include/asm/ftrace.h 56 57 struct dyn_arch_ftrace { 58 struct module *mod; 59 }; 60 #endif /* CONFIG_DYNAMIC_FTRACE */ 61 > 62 #if CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) 63 #define ARCH_HAS_FTRACE_MATCH_ADJUST 64 static inline void arch_ftrace_match_adjust(char **str, char *search) 65 { --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, 2016-04-01 at 00:22 -0300, Thiago Jung Bauermann wrote: > In the ppc64 big endian ABI, function symbols point to function > descriptors. The symbols which point to the function entry points > have a dot in front of the function name. Consequently, when the > ftrace filter mechanism searches for the symbol corresponding to > an entry point address, it gets the dot symbol. > > As a result, ftrace filter users have to be aware of this ABI detail on > ppc64 and prepend a dot to the function name when setting the filter. > > The perf probe command insulates the user from this by ignoring the dot > in front of the symbol name when matching function names to symbols, > but the sysfs interface does not. This patch makes the ftrace filter > mechanism do the same when searching symbols. > > Fixes the following failure in ftracetest's kprobe_ftrace.tc: > > .../kprobe_ftrace.tc: line 9: echo: write error: Invalid argument > > That failure is on this line of kprobe_ftrace.tc: > > echo _do_fork > set_ftrace_filter > > This is because there's no _do_fork entry in the functions list: > > # cat available_filter_functions | grep _do_fork > ._do_fork > > This change introduces no regressions on the perf and ftracetest > testsuite results. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/ftrace.h | 9 +++++++++ > kernel/trace/ftrace.c | 13 +++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h > index 50ca7585abe2..68f1858796c6 100644 > --- a/arch/powerpc/include/asm/ftrace.h > +++ b/arch/powerpc/include/asm/ftrace.h > @@ -58,6 +58,15 @@ struct dyn_arch_ftrace { > struct module *mod; > }; > #endif /* CONFIG_DYNAMIC_FTRACE */ > + > +#if CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) > +#define ARCH_HAS_FTRACE_MATCH_ADJUST I *think* the consenus these days is that we don't add ARCH_HAS #defines in headers. Instead you should either: - add a CONFIG_HAVE_ARCH_FOO and use that - use the #define foo foo trick The latter being that you do: static inline void arch_ftrace_match_adjust(char **str, char *search) { ... } #define arch_ftrace_match_adjust arch_ftrace_match_adjust And in ftrace.c: #ifndef arch_ftrace_match_adjust static inline void arch_ftrace_match_adjust(char **str, char *search) {} #endif Presumably Steve will have a preference for which style you use. > +static inline void arch_ftrace_match_adjust(char **str, char *search) > +{ > + if ((*str)[0] == '.' && search[0] != '.') > + (*str)++; > +} > +#endif /* CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) */ > #endif /* __ASSEMBLY__ */ It seems unfortunate that we need to introduce yet another mechanism to deal with dot symbols. But I guess none of the existing mechanisms work, eg. kprobe_lookup_name(). > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index b1870fbd2b67..e806c2a3b7a8 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -3444,11 +3444,24 @@ struct ftrace_glob { > int type; > }; > > +#ifndef ARCH_HAS_FTRACE_MATCH_ADJUST > +/* > + * If symbols in an architecture don't correspond exactly to the user-visible > + * name of what they represent, it is possible to define this function to > + * perform the necessary adjustments. > +*/ > +static inline void arch_ftrace_match_adjust(char **str, char *search) > +{ > +} > +#endif > + > static int ftrace_match(char *str, struct ftrace_glob *g) > { > int matched = 0; > int slen; > > + arch_ftrace_match_adjust(&str, g->search); I think this would less magical if it didn't modify str directly, instead doing: str = arch_ftrace_match_adjust(str, g->search); And arch_ftrace_match_adjust() would return the adjusted char *. That would mean the generic version would need to return str rather than being empty. cheers
On Thu, 14 Apr 2016 17:11:35 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote: > > > > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h > > index 50ca7585abe2..68f1858796c6 100644 > > --- a/arch/powerpc/include/asm/ftrace.h > > +++ b/arch/powerpc/include/asm/ftrace.h > > @@ -58,6 +58,15 @@ struct dyn_arch_ftrace { > > struct module *mod; > > }; > > #endif /* CONFIG_DYNAMIC_FTRACE */ > > + > > +#if CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) > > +#define ARCH_HAS_FTRACE_MATCH_ADJUST > > I *think* the consenus these days is that we don't add ARCH_HAS #defines in > headers. Instead you should either: > - add a CONFIG_HAVE_ARCH_FOO and use that > - use the #define foo foo trick > > The latter being that you do: > > static inline void arch_ftrace_match_adjust(char **str, char *search) > { > ... > } > #define arch_ftrace_match_adjust arch_ftrace_match_adjust > > And in ftrace.c: > > #ifndef arch_ftrace_match_adjust > static inline void arch_ftrace_match_adjust(char **str, char *search) {} > #endif > > > Presumably Steve will have a preference for which style you use. Actually, what I usually do is simply make a "weak" stub function and let the arch override it. > > > +static inline void arch_ftrace_match_adjust(char **str, char *search) > > +{ > > + if ((*str)[0] == '.' && search[0] != '.') > > + (*str)++; > > +} > > +#endif /* CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) */ > > #endif /* __ASSEMBLY__ */ > > It seems unfortunate that we need to introduce yet another mechanism to deal > with dot symbols. But I guess none of the existing mechanisms work, eg. > kprobe_lookup_name(). What about making a generic conversion to function name, like: arch_sane_function_name(str); Where all sane archs simply return the string name and powerpc can strip the '.' prefix. ;-) Of course that would require: sane_str = arch_sane_function(str); sane_search = arch_sane_function(g->search); and then compare sane_str with sane_search. > > > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index b1870fbd2b67..e806c2a3b7a8 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -3444,11 +3444,24 @@ struct ftrace_glob { > > int type; > > }; > > > > +#ifndef ARCH_HAS_FTRACE_MATCH_ADJUST > > +/* > > + * If symbols in an architecture don't correspond exactly to the user-visible > > + * name of what they represent, it is possible to define this function to > > + * perform the necessary adjustments. > > +*/ > > +static inline void arch_ftrace_match_adjust(char **str, char *search) > > +{ > > +} > > +#endif > > + > > static int ftrace_match(char *str, struct ftrace_glob *g) > > { > > int matched = 0; > > int slen; > > > > + arch_ftrace_match_adjust(&str, g->search); > > I think this would less magical if it didn't modify str directly, instead doing: > > str = arch_ftrace_match_adjust(str, g->search); > > And arch_ftrace_match_adjust() would return the adjusted char *. > > That would mean the generic version would need to return str rather than being > empty. I agree, because if we need to free the string passed it, the function would modify the pointer and we wouldn't be able to free it. In those cases we could do: tmp_str = arch_ftrace_match_adjust(str, search); /* use tmp_str and then ignore */ kfree(str); ** Disclaimer ** Note, I just took the red-eye (2 hours of sleep on the plane) and waiting for my next flight. My focus may be off in this email. -- Steve
Am Donnerstag, 14 April 2016, 06:58:05 schrieb Steven Rostedt: > On Thu, 14 Apr 2016 17:11:35 +1000 > > Michael Ellerman <mpe@ellerman.id.au> wrote: > > Presumably Steve will have a preference for which style you use. > > Actually, what I usually do is simply make a "weak" stub function and > let the arch override it. Ok, in the next version of the patch I'll use the weak function alternative. > > It seems unfortunate that we need to introduce yet another mechanism to > > deal with dot symbols. But I guess none of the existing mechanisms > > work, eg. kprobe_lookup_name(). > > What about making a generic conversion to function name, like: > > arch_sane_function_name(str); > > Where all sane archs simply return the string name and powerpc can > strip the '.' prefix. ;-) > > Of course that would require: > > sane_str = arch_sane_function(str); > sane_search = arch_sane_function(g->search); > > and then compare sane_str with sane_search. I had a look at kprobe_lookup_name but it aims to convert a function name to an address while ftrace_match just wants to compare symbol names, so it's not applicable to what ftrace_match is trying to do. It also goes in the opposite direction of the other places which deal with dot symbols that I could find: it adds a dot because it wants to find the dot symbol, while everywhere else the code removes the dot from the name. For that reason, kprobe_lookup_name wouldn't be able to use a generalized solution for working with dot symbol names like arch_sane_function as proposed above. I did find arch__compare_symbol_names in tools/perf/arch/powerpc/util/sym- handling.c which could be used as-is (if I moved it to somewhere which is common to kernel and userspace code) if ftrace_match only matched the full string, but ftrace_match supports doing front, middle and end string matches as well. It looks like those additional comparison modes are not used at all though. If we do want to reuse arch__compare_symbol_names instead of creating yet another arch-specific symbol comparison mechanism, there are two options: 1. remove ftrace_match and use arch__compare_symbol_names directly; 2. extend arch__compare_symbol_names to support front, middle and end matches. What do you think? > > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > > > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > > index b1870fbd2b67..e806c2a3b7a8 100644 > > > --- a/kernel/trace/ftrace.c > > > +++ b/kernel/trace/ftrace.c > > > @@ -3444,11 +3444,24 @@ struct ftrace_glob { > > > > > > int type; > > > > > > }; > > > > > > +#ifndef ARCH_HAS_FTRACE_MATCH_ADJUST > > > +/* > > > + * If symbols in an architecture don't correspond exactly to the > > > user-visible + * name of what they represent, it is possible to > > > define this function to + * perform the necessary adjustments. > > > +*/ > > > +static inline void arch_ftrace_match_adjust(char **str, char *search) > > > +{ > > > +} > > > +#endif > > > + > > > > > > static int ftrace_match(char *str, struct ftrace_glob *g) > > > { > > > > > > int matched = 0; > > > int slen; > > > > > > + arch_ftrace_match_adjust(&str, g->search); > > > > I think this would less magical if it didn't modify str directly, instead doing: > > str = arch_ftrace_match_adjust(str, g->search); > > > > And arch_ftrace_match_adjust() would return the adjusted char *. > > > > That would mean the generic version would need to return str rather than > > being empty. > > I agree, because if we need to free the string passed it, the function > would modify the pointer and we wouldn't be able to free it. In those > cases we could do: > > tmp_str = arch_ftrace_match_adjust(str, search); > /* use tmp_str and then ignore */ > kfree(str); If you decide against either of my alternatives for using arch__compare_symbol_names, I'll change arch_ftrace_match_adjust to work as you suggested above in the next version of this patch. > ** Disclaimer ** > > Note, I just took the red-eye (2 hours of sleep on the plane) and > waiting for my next flight. My focus may be off in this email. Ouch. Thanks for having a look at the patch and responding to my ping!
diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h index 50ca7585abe2..68f1858796c6 100644 --- a/arch/powerpc/include/asm/ftrace.h +++ b/arch/powerpc/include/asm/ftrace.h @@ -58,6 +58,15 @@ struct dyn_arch_ftrace { struct module *mod; }; #endif /* CONFIG_DYNAMIC_FTRACE */ + +#if CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) +#define ARCH_HAS_FTRACE_MATCH_ADJUST +static inline void arch_ftrace_match_adjust(char **str, char *search) +{ + if ((*str)[0] == '.' && search[0] != '.') + (*str)++; +} +#endif /* CONFIG_PPC64 && (!defined(_CALL_ELF) || _CALL_ELF != 2) */ #endif /* __ASSEMBLY__ */ #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index b1870fbd2b67..e806c2a3b7a8 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -3444,11 +3444,24 @@ struct ftrace_glob { int type; }; +#ifndef ARCH_HAS_FTRACE_MATCH_ADJUST +/* + * If symbols in an architecture don't correspond exactly to the user-visible + * name of what they represent, it is possible to define this function to + * perform the necessary adjustments. +*/ +static inline void arch_ftrace_match_adjust(char **str, char *search) +{ +} +#endif + static int ftrace_match(char *str, struct ftrace_glob *g) { int matched = 0; int slen; + arch_ftrace_match_adjust(&str, g->search); + switch (g->type) { case MATCH_FULL: if (strcmp(str, g->search) == 0)
In the ppc64 big endian ABI, function symbols point to function descriptors. The symbols which point to the function entry points have a dot in front of the function name. Consequently, when the ftrace filter mechanism searches for the symbol corresponding to an entry point address, it gets the dot symbol. As a result, ftrace filter users have to be aware of this ABI detail on ppc64 and prepend a dot to the function name when setting the filter. The perf probe command insulates the user from this by ignoring the dot in front of the symbol name when matching function names to symbols, but the sysfs interface does not. This patch makes the ftrace filter mechanism do the same when searching symbols. Fixes the following failure in ftracetest's kprobe_ftrace.tc: .../kprobe_ftrace.tc: line 9: echo: write error: Invalid argument That failure is on this line of kprobe_ftrace.tc: echo _do_fork > set_ftrace_filter This is because there's no _do_fork entry in the functions list: # cat available_filter_functions | grep _do_fork ._do_fork This change introduces no regressions on the perf and ftracetest testsuite results. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> --- arch/powerpc/include/asm/ftrace.h | 9 +++++++++ kernel/trace/ftrace.c | 13 +++++++++++++ 2 files changed, 22 insertions(+)