* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 6:35 ` [PATCH] tracing: properly align linker defined symbols Sam Ravnborg
@ 2010-07-10 10:18 ` Zeev Tarantov
2010-07-10 11:34 ` Steven Rostedt
2010-07-10 13:49 ` Sam Ravnborg
2010-07-10 22:25 ` Linus Torvalds
` (2 subsequent siblings)
3 siblings, 2 replies; 25+ messages in thread
From: Zeev Tarantov @ 2010-07-10 10:18 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Steven Rostedt, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam@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@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@gmail.com>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@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@gmail.com>
-Zeev
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 10:18 ` Zeev Tarantov
@ 2010-07-10 11:34 ` Steven Rostedt
2010-07-10 13:06 ` Zeev Tarantov
2010-07-10 13:48 ` Sam Ravnborg
2010-07-10 13:49 ` Sam Ravnborg
1 sibling, 2 replies; 25+ messages in thread
From: Steven Rostedt @ 2010-07-10 11:34 UTC (permalink / raw)
To: Zeev Tarantov
Cc: Sam Ravnborg, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote:
> On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <sam@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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 11:34 ` Steven Rostedt
@ 2010-07-10 13:06 ` Zeev Tarantov
2010-07-10 13:48 ` Sam Ravnborg
1 sibling, 0 replies; 25+ messages in thread
From: Zeev Tarantov @ 2010-07-10 13:06 UTC (permalink / raw)
To: rostedt
Cc: Sam Ravnborg, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
On Sat, Jul 10, 2010 at 14:34, Steven Rostedt <rostedt@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@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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 11:34 ` Steven Rostedt
2010-07-10 13:06 ` Zeev Tarantov
@ 2010-07-10 13:48 ` Sam Ravnborg
2010-07-10 15:36 ` Zeev Tarantov
1 sibling, 1 reply; 25+ messages in thread
From: Sam Ravnborg @ 2010-07-10 13:48 UTC (permalink / raw)
To: Steven Rostedt
Cc: Zeev Tarantov, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
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@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;
}
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 13:48 ` Sam Ravnborg
@ 2010-07-10 15:36 ` Zeev Tarantov
0 siblings, 0 replies; 25+ messages in thread
From: Zeev Tarantov @ 2010-07-10 15:36 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Steven Rostedt, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
On Sat, Jul 10, 2010 at 16:48, Sam Ravnborg <sam@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@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@gmail.com>
-Zeev
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 10:18 ` Zeev Tarantov
2010-07-10 11:34 ` Steven Rostedt
@ 2010-07-10 13:49 ` Sam Ravnborg
1 sibling, 0 replies; 25+ messages in thread
From: Sam Ravnborg @ 2010-07-10 13:49 UTC (permalink / raw)
To: Zeev Tarantov
Cc: Steven Rostedt, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
>
> 2.6.35-rc4 from tarball patched with only this, same config & same
> compiler boots fine.
>
> Tested-by: Zeev Tarantov <zeev.tarantov@gmail.com>
Thanks for your paitient testing Zeev!
Sam
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 6:35 ` [PATCH] tracing: properly align linker defined symbols Sam Ravnborg
2010-07-10 10:18 ` Zeev Tarantov
@ 2010-07-10 22:25 ` Linus Torvalds
2010-07-10 22:27 ` Linus Torvalds
2010-07-11 6:07 ` Sam Ravnborg
2010-07-15 15:05 ` Sam Ravnborg
2010-07-22 11:51 ` [tip:perf/urgent] tracing: Properly " tip-bot for Sam Ravnborg
3 siblings, 2 replies; 25+ messages in thread
From: Linus Torvalds @ 2010-07-10 22:25 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Steven Rostedt, Zeev Tarantov, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]
On Fri, Jul 9, 2010 at 11:35 PM, Sam Ravnborg <sam@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?
Linus
>
> 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@gmail.com>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@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
>
>
[-- Attachment #2: diff --]
[-- Type: application/octet-stream, Size: 3349 bytes --]
include/asm-generic/vmlinux.lds.h | 52 +++++++++++++++----------------------
1 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 48c5299..b91a555 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -91,51 +91,50 @@
#define MEM_DISCARD(sec) *(.mem##sec)
#endif
+#define SYMBOL_SECTION(name, section) \
+ . = ALIGN(32); \
+ VMLINUX_SYMBOL(__start_##section) = .; \
+ *(name) \
+ VMLINUX_SYMBOL(__stop_##section) = .;
+
#ifdef CONFIG_FTRACE_MCOUNT_RECORD
-#define MCOUNT_REC() . = ALIGN(8); \
- VMLINUX_SYMBOL(__start_mcount_loc) = .; \
- *(__mcount_loc) \
- VMLINUX_SYMBOL(__stop_mcount_loc) = .;
+#define MCOUNT_REC() \
+ SYMBOL_SECTION(__mcount_lock, mcount_loc)
#else
#define MCOUNT_REC()
#endif
#ifdef CONFIG_TRACE_BRANCH_PROFILING
-#define LIKELY_PROFILE() VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
- *(_ftrace_annotated_branch) \
- VMLINUX_SYMBOL(__stop_annotated_branch_profile) = .;
+#define LIKELY_PROFILE() \
+ SYMBOL_SECTION(_ftrace_annotated_branch, annotated_branch_profile)
#else
#define LIKELY_PROFILE()
#endif
#ifdef CONFIG_PROFILE_ALL_BRANCHES
-#define BRANCH_PROFILE() VMLINUX_SYMBOL(__start_branch_profile) = .; \
- *(_ftrace_branch) \
- VMLINUX_SYMBOL(__stop_branch_profile) = .;
+#define BRANCH_PROFILE() \
+ SYMBOL_SECTION(_ftrace_branch, branch_profile)
#else
#define BRANCH_PROFILE()
#endif
#ifdef CONFIG_EVENT_TRACING
-#define FTRACE_EVENTS() VMLINUX_SYMBOL(__start_ftrace_events) = .; \
- *(_ftrace_events) \
- VMLINUX_SYMBOL(__stop_ftrace_events) = .;
+#define FTRACE_EVENTS() \
+ SYMBOL_SECTION(_ftrace_events, ftrace_events)
#else
#define FTRACE_EVENTS()
#endif
#ifdef CONFIG_TRACING
-#define TRACE_PRINTKS() VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .; \
- *(__trace_printk_fmt) /* Trace_printk fmt' pointer */ \
- VMLINUX_SYMBOL(__stop___trace_bprintk_fmt) = .;
+#define TRACE_PRINTKS() \
+ SYMBOL_SECTION(__trace_printk_fmt, __trace_bprintk_fmt)
#else
#define TRACE_PRINTKS()
#endif
#ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
- *(__syscalls_metadata) \
- VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
+#define TRACE_SYSCALLS() \
+ SYMBOL_SECTION(__syscalls_metadata, syscalls_metadata)
#else
#define TRACE_SYSCALLS()
#endif
@@ -150,19 +149,10 @@
CPU_KEEP(exit.data) \
MEM_KEEP(init.data) \
MEM_KEEP(exit.data) \
- . = ALIGN(8); \
- VMLINUX_SYMBOL(__start___markers) = .; \
- *(__markers) \
- VMLINUX_SYMBOL(__stop___markers) = .; \
- . = ALIGN(32); \
- VMLINUX_SYMBOL(__start___tracepoints) = .; \
- *(__tracepoints) \
- VMLINUX_SYMBOL(__stop___tracepoints) = .; \
+ SYMBOL_SECTION(__markers, __markers) \
+ SYMBOL_SECTION(__tracepoints, __tracepoints); \
/* implement dynamic printk debug */ \
- . = ALIGN(8); \
- VMLINUX_SYMBOL(__start___verbose) = .; \
- *(__verbose) \
- VMLINUX_SYMBOL(__stop___verbose) = .; \
+ SYMBOL_SECTION(__verbose, __verbose) \
LIKELY_PROFILE() \
BRANCH_PROFILE() \
TRACE_PRINTKS() \
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 22:25 ` Linus Torvalds
@ 2010-07-10 22:27 ` Linus Torvalds
2010-07-11 6:07 ` Sam Ravnborg
1 sibling, 0 replies; 25+ messages in thread
From: Linus Torvalds @ 2010-07-10 22:27 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Steven Rostedt, Zeev Tarantov, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
On Sat, Jul 10, 2010 at 3:25 PM, Linus Torvalds
<torvalds@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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 22:25 ` Linus Torvalds
2010-07-10 22:27 ` Linus Torvalds
@ 2010-07-11 6:07 ` Sam Ravnborg
1 sibling, 0 replies; 25+ messages in thread
From: Sam Ravnborg @ 2010-07-11 6:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Steven Rostedt, Zeev Tarantov, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
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@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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-10 6:35 ` [PATCH] tracing: properly align linker defined symbols Sam Ravnborg
2010-07-10 10:18 ` Zeev Tarantov
2010-07-10 22:25 ` Linus Torvalds
@ 2010-07-15 15:05 ` Sam Ravnborg
2010-07-15 15:15 ` Steven Rostedt
2010-07-22 11:51 ` [tip:perf/urgent] tracing: Properly " tip-bot for Sam Ravnborg
3 siblings, 1 reply; 25+ messages in thread
From: Sam Ravnborg @ 2010-07-15 15:05 UTC (permalink / raw)
To: Steven Rostedt, Zeev Tarantov
Cc: Linus Torvalds, LKML, Ingo Molnar, Frederic Weisbecker,
Andrew Morton, Rafael J. Wysocki
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@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@gmail.com>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@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
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-15 15:05 ` Sam Ravnborg
@ 2010-07-15 15:15 ` Steven Rostedt
2010-07-15 18:31 ` Sam Ravnborg
0 siblings, 1 reply; 25+ messages in thread
From: Steven Rostedt @ 2010-07-15 15:15 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Zeev Tarantov, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] tracing: properly align linker defined symbols
2010-07-15 15:15 ` Steven Rostedt
@ 2010-07-15 18:31 ` Sam Ravnborg
0 siblings, 0 replies; 25+ messages in thread
From: Sam Ravnborg @ 2010-07-15 18:31 UTC (permalink / raw)
To: Steven Rostedt
Cc: Zeev Tarantov, Linus Torvalds, LKML, Ingo Molnar,
Frederic Weisbecker, Andrew Morton, Rafael J. Wysocki
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tip:perf/urgent] tracing: Properly align linker defined symbols
2010-07-10 6:35 ` [PATCH] tracing: properly align linker defined symbols Sam Ravnborg
` (2 preceding siblings ...)
2010-07-15 15:05 ` Sam Ravnborg
@ 2010-07-22 11:51 ` tip-bot for Sam Ravnborg
3 siblings, 0 replies; 25+ messages in thread
From: tip-bot for Sam Ravnborg @ 2010-07-22 11:51 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, zeev.tarantov, sam, fweisbec, rostedt,
tglx
Commit-ID: 07fca0e57fca925032526349f4370f97ed580cc9
Gitweb: http://git.kernel.org/tip/07fca0e57fca925032526349f4370f97ed580cc9
Author: Sam Ravnborg <sam@ravnborg.org>
AuthorDate: Sat, 10 Jul 2010 08:35:00 +0200
Committer: Steven Rostedt <rostedt@goodmis.org>
CommitDate: Tue, 20 Jul 2010 19:02:52 -0400
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@gmail.com>
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
LKML-Reference: <20100710063459.GA14596@merkur.ravnborg.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
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()
/*
^ permalink raw reply related [flat|nested] 25+ messages in thread