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: Linus Torvalds on 10 Jul 2010 18:30 On Sat, Jul 10, 2010 at 3:25 PM, Linus Torvalds <torvalds(a)linux-foundation.org> wrote: > > Here's an example patch. Untested. Whatever. Ok, and by "untested", I clearly mean just that. I see a typo immediately, but you get the idea. +#define MCOUNT_REC() \ + SYMBOL_SECTION(__mcount_lock, mcount_loc) I'm too used to typing "lock", that __mcount_lock thing should obviously be "__mcount_loc" So take the patch as the RFC it is, and fix at least that typo before actually testing it. Linus -- 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 11 Jul 2010 02:10 On Sat, Jul 10, 2010 at 03:25:02PM -0700, Linus Torvalds wrote: > On Fri, Jul 9, 2010 at 11:35 PM, Sam Ravnborg <sam(a)ravnborg.org> wrote: > > > > 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. > > Ok, why not clean this up a bit more, and use a helper macro for this > pattern. There's a fair number of users of that kind of pattern, so > that actually removes a few lines. > > Here's an example patch. Untested. Whatever. But just this part > > 1 files changed, 21 insertions(+), 31 deletions(-) > > says to me that it's a good idea, and there are other cases that could > use the new SYMBOL_SECTION() helper. > > What do people think? Looks good. I especially like how we with this standardize on the alignment. I will make sure a working version hits next merge window. A few comments. +#define SYMBOL_SECTION(name, section) \ + . = ALIGN(32); \ + VMLINUX_SYMBOL(__start_##section) = .; \ + *(name) \ + VMLINUX_SYMBOL(__stop_##section) = .; The arguments to this macro is confusing. Something like this: #define SYMBOL_SECTION(section, symbol_suffix) To encourage people to use the section name as suffix the __start / __stop variables we could introduce an additional define: #define SYMBOL_SECTION(section) SYMBOL_SECTION_SUFFIX(section, section) #define SYMBOL_SECTION_SUFFIX(section, symbol_suffix) \ + . = ALIGN(32); \ + VMLINUX_SYMBOL(__start_##symbol_suffix) = .; \ + *(section) \ + VMLINUX_SYMBOL(__stop_##symbol_suffix) = .; I will update the patch to reflect this (+ the fix you pointed out). But it will wait until Steven has decided what patch to forward to fix the discussed regression. Sam -- 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 15 Jul 2010 11:10 On Sat, Jul 10, 2010 at 08:34:59AM +0200, Sam Ravnborg 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 Steven - Zeev reported that this fixed the boot problem. What is next step? Do you forward this patch or do you prefer another fix? 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 > -- 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 15 Jul 2010 11:20 On Thu, 2010-07-15 at 17:05 +0200, Sam Ravnborg wrote: > On Sat, Jul 10, 2010 at 08:34:59AM +0200, Sam Ravnborg 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 > > Steven - Zeev reported that this fixed the boot problem. > What is next step? > Do you forward this patch or do you prefer another fix? > Hi Sam, I'm currently at OLS (yes it still exists!) I'll test it and send off this fix to Ingo when I get back on Monday, as I believe Linus did prefer this one. -- 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: Sam Ravnborg on 15 Jul 2010 14:40
On Thu, Jul 15, 2010 at 11:15:59AM -0400, Steven Rostedt wrote: > On Thu, 2010-07-15 at 17:05 +0200, Sam Ravnborg wrote: > > On Sat, Jul 10, 2010 at 08:34:59AM +0200, Sam Ravnborg 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 > > > > Steven - Zeev reported that this fixed the boot problem. > > What is next step? > > Do you forward this patch or do you prefer another fix? > > > > Hi Sam, > > I'm currently at OLS (yes it still exists!) I'll test it and send off > this fix to Ingo when I get back on Monday, as I believe Linus did > prefer this one. Great. I'm most likely away from mail the next week so do not expect prompt responses from me. Sam -- 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/ |