[4/6] ftrace syscalls: Allow arch specific syscall symbol matching

From: Ian Munsie <imunsie@au1.ibm.com>

Some architectures have unusual symbol names and the generic code to
match the symbol name with the function name for the syscall metadata
will fail. For example, symbols on PPC64 start with a period and the
generic code will fail to match them.

This patch moves the match logic out into a macro which can be
overridden by defining arch_syscall_match_sym_name in an archs
asm/ftrace.h if needed.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 Documentation/trace/ftrace-design.txt |    4 ++++
 include/linux/ftrace.h                |    9 +++++++++
 kernel/trace/trace_syscalls.c         |    8 +-------
 3 files changed, 14 insertions(+), 7 deletions(-)

On Wed, 2011-02-02 at 18:11 +1100, Ian Munsie wrote:

I'll answer your question here.

> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index dcd6a7c..0d0e109 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -527,6 +527,15 @@ extern enum ftrace_dump_mode ftrace_dump_on_oops;
>  #ifdef CONFIG_FTRACE_SYSCALLS
>  
>  unsigned long arch_syscall_addr(int nr);
> +#ifndef arch_syscall_match_sym_name
> +/*
> + * Only compare after the "sys" prefix. Archs that use
> + * syscall wrappers may have syscalls symbols aliases prefixed
> + * with "SyS" instead of "sys", leading to an unwanted
> + * mismatch.
> + */
> +#define arch_syscall_match_sym_name(sym, name) !strcmp(sym + 3, name + 3)

Instead, you could have:

#ifndef ARCH_HAS_SYSCALL_MATCH_SYM_NAME

static inline arch_syscall_match_sym_name(const char *sym, const char *name)
{
	return strcmp(sym + 3, name + 3) != 0;
}


If an arch needs to make its own, then it can simply override it by
creating its own version and defining:

#define ARCH_HAS_SYSCALL_MATCH_SYM_NAME

Just like they do when an arch has its own strcmp.

This just keeps things cleaner. I like to avoid macros as they can have
nasty side effects (never would have guess that if you look at what I've
done in trace/ftrace.h ;-)

For example, if we use your call as:

arch_syscall_match_sym_name(str = sym[5], name);

That str would end up being something unexpected. I'm not condoning such
side-effect code, but it is something to think about when using macros.

-- Steve


> +#endif
>  
>  #endif /* CONFIG_FTRACE_SYSCALLS */
>  
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 33360b9..76bffba 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -72,13 +72,7 @@ static struct syscall_metadata *find_syscall_meta(unsigned long syscall)
>  	kallsyms_lookup(syscall, NULL, NULL, NULL, str);
>  
>  	for ( ; start < stop; start++) {
> -		/*
> -		 * Only compare after the "sys" prefix. Archs that use
> -		 * syscall wrappers may have syscalls symbols aliases prefixed
> -		 * with "SyS" instead of "sys", leading to an unwanted
> -		 * mismatch.
> -		 */
> -		if (start->name && !strcmp(start->name + 3, str + 3))
> +		if (start->name && arch_syscall_match_sym_name(str, start->name))
>  			return start;
>  	}
>  	return NULL;

> +#define arch_syscall_match_sym_name(sym, name) !strcmp(sym + 3, name
+ 3)

Whenever you use a #define macro arg, you should enclose it in ().
About the only time you don't need to is when it is being
passed as an argument to another function
(ie when it's use is also ',' separated).

So the above ought to be:
#define arch_syscall_match_sym_name(sym, name) (!strcmp((sym) + 3,
(name) + 3))

Whether an inline function is better or worse is much more subtle!
For instance I've used:
   asm volatile ( "# line " STR(__LINE__) :: )
to stop gcc merging the tails of conditionals.
Useful when the conditional is at the end of a loop (etc),
it might increase code size slightly, but removes a branch.

If I put one of those in an 'inline' function separate copies
of the function end up sharing code.
With a #define __LINE__ differs so they don't.

(I had some code to get below 190 clocks, these changes
were significant!)

	David

On Wed, 2011-02-02 at 14:15 +0000, David Laight wrote:
> > +#define arch_syscall_match_sym_name(sym, name) !strcmp(sym + 3, name
> + 3)
> 
> Whenever you use a #define macro arg, you should enclose it in ().
> About the only time you don't need to is when it is being
> passed as an argument to another function
> (ie when it's use is also ',' separated).
> 
> So the above ought to be:
> #define arch_syscall_match_sym_name(sym, name) (!strcmp((sym) + 3,
> (name) + 3))

I would have mentioned this if I wanted it to stay a macro ;)

> 
> Whether an inline function is better or worse is much more subtle!
> For instance I've used:
>    asm volatile ( "# line " STR(__LINE__) :: )
> to stop gcc merging the tails of conditionals.
> Useful when the conditional is at the end of a loop (etc),
> it might increase code size slightly, but removes a branch.
> 
> If I put one of those in an 'inline' function separate copies
> of the function end up sharing code.
> With a #define __LINE__ differs so they don't.
> 
> (I had some code to get below 190 clocks, these changes
> were significant!)

For what you were doing, this may have helped. But the code in question
is the "default" version of the function. I much more prefer it to be a
static inline. The issues you experience could change from gcc to gcc.
But static inlined functions are much cleaner and easier to read than
macros.

Using a macro for this purpose is just too messy.

Again, look at include/trace/ftrace.h. If I'm saying using a macro is
ugly, then don't use it! Listen to me, because I'm Mr. Ugly Macro Man.

-- Steve

Excerpts from Steven Rostedt's message of Thu Feb 03 01:04:44 +1100 2011:
> I'll answer your question here.

> > +#define arch_syscall_match_sym_name(sym, name) !strcmp(sym + 3, name + 3)
> 
> Instead, you could have:
> 
> #ifndef ARCH_HAS_SYSCALL_MATCH_SYM_NAME
> 
> static inline arch_syscall_match_sym_name(const char *sym, const char *name)
> {
>     return strcmp(sym + 3, name + 3) != 0;
> }
> 
> 
> If an arch needs to make its own, then it can simply override it by
> creating its own version and defining:
> 
> #define ARCH_HAS_SYSCALL_MATCH_SYM_NAME
> 
> Just like they do when an arch has its own strcmp.

Ok, I've changed it over. Just doing a quick regression test on ppc64 &
x86 then I'll repost.

Cheers,
-Ian