Re: [PATCH 1/2] tracing: Move syscalls metadata handling from arch to core

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Sep 19, 2009 at 09:48:16AM +0200, Heiko Carstens wrote:
> On Sat, Sep 19, 2009 at 07:39:16AM +0200, Frederic Weisbecker wrote:
> > Most of the syscalls metadata processing is done from arch.
> > But these operations are mostly generic accross archs. Especially now
> > that we have a common variable name that expresses the number of
> > syscalls supported by an arch: NR_syscalls, the only remaining bits
> > that need to reside in arch is the syscall nr to addr translation.
> 
> That won't work in its current form, since there is a small difference
> between x86 and s390:
> 
> > diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
> > index 57bdcb1..7c5752c 100644
> > --- a/arch/s390/kernel/ftrace.c
> > +++ b/arch/s390/kernel/ftrace.c
> > -static struct syscall_metadata *find_syscall_meta(unsigned long syscall)
> > -{
> > -	struct syscall_metadata *start;
> > -	struct syscall_metadata *stop;
> > -	char str[KSYM_SYMBOL_LEN];
> > -
> > -	start = (struct syscall_metadata *)__start_syscalls_metadata;
> > -	stop = (struct syscall_metadata *)__stop_syscalls_metadata;
> > -	kallsyms_lookup(syscall, NULL, NULL, NULL, str);
> > -
> > -	for ( ; start < stop; start++) {
> > -		if (start->name && !strcmp(start->name + 3, str + 3))
>                                            ^^^^^^^^^^^^^^^^^^^^^^^^



Oh, I thought it was to zap the "sys" prefix comparison.



> > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > index 9dbb527..4adb867 100644
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > -static struct syscall_metadata *find_syscall_meta(unsigned long *syscall)
> > -{
> > -	struct syscall_metadata *start;
> > -	struct syscall_metadata *stop;
> > -	char str[KSYM_SYMBOL_LEN];
> > -
> > -
> > -	start = (struct syscall_metadata *)__start_syscalls_metadata;
> > -	stop = (struct syscall_metadata *)__stop_syscalls_metadata;
> > -	kallsyms_lookup((unsigned long) syscall, NULL, NULL, NULL, str);
> > -
> > -	for ( ; start < stop; start++) {
> > -		if (start->name && !strcmp(start->name, str))
>                                            ^^^^^^^^^^^^^^^^
> > -			return start;
> > -	}
> > -	return NULL;
> 
> The reason for the "+ 3 " is that architectures with syscall wrappers have
> alias function names which also show up in kallsysms:
> 
> 000000000001c788 t show_cpuinfo
> 000000000001c9e0 T SyS_s390_personality
> 000000000001c9e0 T sys_s390_personality
> 000000000001ca48 T SyS_s390_newuname
> 000000000001ca48 T sys_s390_newuname
> 000000000001cac8 T SyS_ipc
> 000000000001cac8 T sys_ipc
> 000000000001cd00 T SyS_s390_old_mmap
> 000000000001cd00 T sys_s390_old_mmap
> 000000000001ce68 T SyS_mmap2
> 000000000001ce68 T sys_mmap2
> 000000000001cfc4 t FixPerRegisters
> 
> So kallsyms_lookup(...) would currently always return a string that starts
> with "SyS" instead of "sys". Since the metadata syscall names start with
> "sys" there is no match.
> If you could change the generic version so it also contains a "+ 3" it
> should work for all architectures.
> Might be worth a comment that I didn't add back then :)



Ah ok. So the fix is easy.
Thanks for the tip!


> Hmm... maybe it's even possible to throw out the "SyS" variants out of
> the kallsyms table and only keep the alias names?
> That would shrink the kernel image a bit.


Yeah, unless someone finds strong reasons to keep them.

Thanks!