tracing: properly align linker defined symbols
in [Kernel]
|
Prev: [patch] trace: strlen() return doesn't account for the NULL
Next: [patch] cgroups: save space for the terminator
From: Zeev Tarantov on 10 Jul 2010 06:20 On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam(a)ravnborg.org> wrote: > Zeev - please try this replacement patch. > The alignmnet is increased to 32 bytes compared to my previous version and > we introduce alignmnet for ftrace_events too. > > � � � �Sam > > From 40bedb8fda25d2cf9ecdd41ab48a24104607c37e Mon Sep 17 00:00:00 2001 > From: Sam Ravnborg <sam(a)ravnborg.org> > Date: Sat, 10 Jul 2010 08:24:12 +0200 > Subject: [PATCH] tracing: properly align linker defined symbols > > We define a number of symbols in the linker scipt like this: > > � �__start_syscalls_metadata = .; > � �*(__syscalls_metadata) > > But we do not know the alignment of "." when we assign > the __start_syscalls_metadata symbol. > gcc started to uses bigger alignment for structs (32 bytes), > so we saw situations where the linker due to alignment > constraints increased the value of "." after the symbol assignment. > > This resulted in boot fails. > > Fix this by forcing a 32 byte alignment of "." before the > assignment. > > This patch introduces the forced alignment for > ftrace_events and syscalls_metadata. > It may be required in more places. > > Reported-by: Zeev Tarantov <zeev.tarantov(a)gmail.com> > Signed-off-by: Sam Ravnborg <sam(a)ravnborg.org> > Cc: Steven Rostedt <rostedt(a)goodmis.org> > Cc: Frederic Weisbecker <fweisbec(a)gmail.com> > --- > �include/asm-generic/vmlinux.lds.h | � 10 ++++++++++ > �1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > index 48c5299..4b5902a 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -63,6 +63,12 @@ > �/* Align . to a 8 byte boundary equals to maximum function alignment. */ > �#define ALIGN_FUNCTION() �. = ALIGN(8) > > +/* > + * Align to a 32 byte boundary equal to the > + * alignment gcc 4.5 uses for a struct > + */ > +#define STRUCT_ALIGN() . = ALIGN(32) > + > �/* The actual configuration determine if the init/exit sections > �* are handled as text/data or they can be discarded (which > �* often happens at runtime) > @@ -166,7 +172,11 @@ > � � � �LIKELY_PROFILE() � � � � � � � � � � � � � � � � � � � � � � � �\ > � � � �BRANCH_PROFILE() � � � � � � � � � � � � � � � � � � � � � � � �\ > � � � �TRACE_PRINTKS() � � � � � � � � � � � � � � � � � � � � � � � � \ > + � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � \ > + � � � STRUCT_ALIGN(); � � � � � � � � � � � � � � � � � � � � � � � � \ > � � � �FTRACE_EVENTS() � � � � � � � � � � � � � � � � � � � � � � � � \ > + � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � � \ > + � � � STRUCT_ALIGN(); � � � � � � � � � � � � � � � � � � � � � � � � \ > � � � �TRACE_SYSCALLS() > > �/* > -- > 1.6.0.6 > > 2.6.35-rc4 from tarball patched with only this, same config & same compiler boots fine. Tested-by: Zeev Tarantov <zeev.tarantov(a)gmail.com> -Zeev -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 10 Jul 2010 07:40 On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote: > On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam(a)ravnborg.org> wrote: > > +/* > > + * Align to a 32 byte boundary equal to the > > + * alignment gcc 4.5 uses for a struct > > + */ > > +#define STRUCT_ALIGN() . = ALIGN(32) > > + What I'm nervous about is when gcc 4.8 decides to up the alignment to 64. Maybe we should have both patches, just to be safe. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Zeev Tarantov on 10 Jul 2010 09:10 On Sat, Jul 10, 2010 at 14:34, Steven Rostedt <rostedt(a)goodmis.org> wrote: > On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote: >> On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam(a)ravnborg.org> wrote: > >> > +/* >> > + * Align to a 32 byte boundary equal to the >> > + * alignment gcc 4.5 uses for a struct >> > + */ >> > +#define STRUCT_ALIGN() . = ALIGN(32) >> > + > > What I'm nervous about is when gcc 4.8 decides to up the alignment to > 64. > > Maybe we should have both patches, just to be safe. > > -- Steve > I don't want to post obvious or inane suggests to the mailing list, but if you're worried about gcc changing the default alignment, the solution seems to be one of: 1. Not relying on the default alignment and specifying explicitly what you want. 2. Querying the default alignment before compilation, either using an API gcc may provide or (cumbersomely) by testing. -Zeev -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Sam Ravnborg on 10 Jul 2010 09:50 On Sat, Jul 10, 2010 at 07:34:05AM -0400, Steven Rostedt wrote: > On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote: > > On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam(a)ravnborg.org> wrote: > > > > +/* > > > + * Align to a 32 byte boundary equal to the > > > + * alignment gcc 4.5 uses for a struct > > > + */ > > > +#define STRUCT_ALIGN() . = ALIGN(32) > > > + > > What I'm nervous about is when gcc 4.8 decides to up the alignment to > 64. > > Maybe we should have both patches, just to be safe. Another approach could be to just stop playing games with alignment and use the fact that __stop_syscalls_metadata point to next byte after last entry. Something like the below untested patch. But to fix the current regression I prefer the simpler patch that just fixup the aligment. Sam diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c index 34e3580..50e9606 100644 --- a/kernel/trace/trace_syscalls.c +++ b/kernel/trace/trace_syscalls.c @@ -79,15 +79,19 @@ static struct syscall_metadata *find_syscall_meta(unsigned long syscall) stop = (struct syscall_metadata *)__stop_syscalls_metadata; kallsyms_lookup(syscall, NULL, NULL, NULL, str); - for ( ; start < stop; start++) { + /* + * start may point a few bytes before first entry + * stop points at first byte after last entry + */ + for (stop--; stop >= start; stop--) { /* * 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)) - return start; + if (stop->name && !strcmp(stop->name + 3, str + 3)) + return stop; } return NULL; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Zeev Tarantov on 10 Jul 2010 11:40
On Sat, Jul 10, 2010 at 16:48, Sam Ravnborg <sam(a)ravnborg.org> wrote: > On Sat, Jul 10, 2010 at 07:34:05AM -0400, Steven Rostedt wrote: >> On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote: >> > On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam(a)ravnborg.org> wrote: >> >> > > +/* >> > > + * Align to a 32 byte boundary equal to the >> > > + * alignment gcc 4.5 uses for a struct >> > > + */ >> > > +#define STRUCT_ALIGN() . = ALIGN(32) >> > > + >> >> What I'm nervous about is when gcc 4.8 decides to up the alignment to >> 64. >> >> Maybe we should have both patches, just to be safe. > > Another approach could be to just stop playing games with alignment > and use the fact that __stop_syscalls_metadata point to next byte > after last entry. > > Something like the below untested patch. > > But to fix the current regression I prefer the simpler > patch that just fixup the aligment. > > � � � �Sam > > > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c > index 34e3580..50e9606 100644 > --- a/kernel/trace/trace_syscalls.c > +++ b/kernel/trace/trace_syscalls.c > @@ -79,15 +79,19 @@ static struct syscall_metadata *find_syscall_meta(unsigned long syscall) > � � � �stop = (struct syscall_metadata *)__stop_syscalls_metadata; > � � � �kallsyms_lookup(syscall, NULL, NULL, NULL, str); > > - � � � for ( ; start < stop; start++) { > + � � � /* > + � � � �* start may point a few bytes before first entry > + � � � �* stop points at first byte after last entry > + � � � �*/ > + � � � for (stop--; stop >= start; stop--) { > � � � � � � � �/* > � � � � � � � � * 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)) > - � � � � � � � � � � � return start; > + � � � � � � � if (stop->name && !strcmp(stop->name + 3, str + 3)) > + � � � � � � � � � � � return stop; > � � � �} > � � � �return NULL; > �} > This also boots (source from tarball with only this patch, same config & compiler). Tested-by: Zeev Tarantov <zeev.tarantov(a)gmail.com> -Zeev -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |