linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
@ 2016-04-01  3:22 Thiago Jung Bauermann
  2016-04-01 19:51 ` kbuild test robot
  2016-04-14  7:11 ` Michael Ellerman
  0 siblings, 2 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2016-04-01  3:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: linuxppc-dev, Thiago Jung Bauermann, Steven Rostedt, Ingo Molnar,
	Michael Ellerman

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
+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)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-01  3:22 [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64 Thiago Jung Bauermann
@ 2016-04-01 19:51 ` kbuild test robot
  2016-04-01 21:28   ` Thiago Jung Bauermann
  2016-04-14  7:11 ` Michael Ellerman
  1 sibling, 1 reply; 8+ messages in thread
From: kbuild test robot @ 2016-04-01 19:51 UTC (permalink / raw)
  To: Thiago Jung Bauermann
  Cc: kbuild-all, linux-kernel, linuxppc-dev, Thiago Jung Bauermann,
	Steven Rostedt, Ingo Molnar, Michael Ellerman

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 27992 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-01 19:51 ` kbuild test robot
@ 2016-04-01 21:28   ` Thiago Jung Bauermann
  2016-04-14  0:39     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 8+ messages in thread
From: Thiago Jung Bauermann @ 2016-04-01 21:28 UTC (permalink / raw)
  To: linux-kernel
  Cc: kbuild test robot, kbuild-all, linuxppc-dev, Steven Rostedt,
	Ingo Molnar, Michael Ellerman, bauerman

Am Samstag, 02 April 2016, 03:51:21 schrieb kbuild test robot:
> >> 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

I forgot to use defined() in the #if expression. Here’s the fixed version.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


---- 8< ---- 8< ---- 8< ---- 8< ----


>From 27660a3b6c4147f9e1811b103cc47a34a53817c1 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Date: Wed, 30 Mar 2016 21:26:32 -0300
Subject: [PATCH] ftrace: Match dot symbols when searching functions on ppc64

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..f6ed1908f0f7 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 defined(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 /* defined(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)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-01 21:28   ` Thiago Jung Bauermann
@ 2016-04-14  0:39     ` Thiago Jung Bauermann
  2016-04-14  3:51       ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: Thiago Jung Bauermann @ 2016-04-14  0:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: linux-kernel, kbuild test robot, Steven Rostedt, Ingo Molnar, kbuild-all

Hello,

Am Freitag, 01 April 2016, 18:28:06 schrieb Thiago Jung Bauermann:
> Am Samstag, 02 April 2016, 03:51:21 schrieb kbuild test robot:
> > >> 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
> 
> I forgot to use defined() in the #if expression. Here’s the fixed version.

People seem to be considering patches for next, so this looks like a good 
moment to ping about this one.

Ps: patchwork seems to have an issue which causes it to show the message 
body as if it were the commit message, but if you feed my original email 
(the one I’m replying to here) to git am, the commit message will be 
correct.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-14  0:39     ` Thiago Jung Bauermann
@ 2016-04-14  3:51       ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2016-04-14  3:51 UTC (permalink / raw)
  To: Thiago Jung Bauermann
  Cc: linuxppc-dev, linux-kernel, kbuild test robot, Ingo Molnar, kbuild-all

On Wed, 13 Apr 2016 21:39 -0300
Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> wrote:


> People seem to be considering patches for next, so this looks like a good 
> moment to ping about this one.

Your timing is fine with respect to the merge window. I'm currently
traveling, but I'll get to it on Monday. I have it marked as TODO.

> 
> Ps: patchwork seems to have an issue which causes it to show the message 
> body as if it were the commit message, but if you feed my original email 
> (the one I’m replying to here) to git am, the commit message will be 
> correct.
> 

Yeah I noticed that. But I'll be able to handle it.

Thanks,

-- Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-01  3:22 [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64 Thiago Jung Bauermann
  2016-04-01 19:51 ` kbuild test robot
@ 2016-04-14  7:11 ` Michael Ellerman
  2016-04-14 10:58   ` Steven Rostedt
  1 sibling, 1 reply; 8+ messages in thread
From: Michael Ellerman @ 2016-04-14  7:11 UTC (permalink / raw)
  To: Thiago Jung Bauermann, linux-kernel
  Cc: linuxppc-dev, Steven Rostedt, Ingo Molnar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-14  7:11 ` Michael Ellerman
@ 2016-04-14 10:58   ` Steven Rostedt
  2016-04-14 23:48     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2016-04-14 10:58 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Thiago Jung Bauermann, linux-kernel, linuxppc-dev, Ingo Molnar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64.
  2016-04-14 10:58   ` Steven Rostedt
@ 2016-04-14 23:48     ` Thiago Jung Bauermann
  0 siblings, 0 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2016-04-14 23:48 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Michael Ellerman, linux-kernel, linuxppc-dev, Ingo Molnar

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!

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-04-14 23:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-01  3:22 [PATCH] ftrace: filter: Match dot symbols when searching functions on ppc64 Thiago Jung Bauermann
2016-04-01 19:51 ` kbuild test robot
2016-04-01 21:28   ` Thiago Jung Bauermann
2016-04-14  0:39     ` Thiago Jung Bauermann
2016-04-14  3:51       ` Steven Rostedt
2016-04-14  7:11 ` Michael Ellerman
2016-04-14 10:58   ` Steven Rostedt
2016-04-14 23:48     ` Thiago Jung Bauermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).