LinuxLists.cc - [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, Jul 09, 2010 at 03:56:42PM -0400, Steven Rostedt wrote:
>
> Ingo,
>
> Please pull the latest tip/perf/urgent tree, which can be found at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/perf/urgent
>
>
> Steven Rostedt (1):
> tracing: Add alignment to syscall metadata declarations
>
> ----
> include/linux/syscalls.h | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
> ---------------------------
> commit 44a54f787c0abcf75a2ed49b8ec8b2b512468f73
> Author: Steven Rostedt <[email protected]>
> Date: Fri Jul 9 15:41:44 2010 -0400
>
> tracing: Add alignment to syscall metadata declarations
>
> For some reason if we declare a static variable and then assign it
> later, and the assignment contains a __attribute__((__aligned__(#))),
> some versions of gcc will ignore it.
>
> This caused the syscall meta data to not be compact in its section
> and caused a kernel oops when the section was being read.
>
> The fix for these versions of gcc seems to be to add the aligned
> attribute to the declaration as well.
>
> This fixes the BZ regression:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=16353
>
> Reported-by: Zeev Tarantov <[email protected]>
> Tested-by: Zeev Tarantov <[email protected]>
> Acked-by: Frederic Weisbecker <[email protected]>
> LKML-Reference: <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 7f614ce..13ebb54 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -124,7 +124,8 @@ extern struct trace_event_functions enter_syscall_print_funcs;
> extern struct trace_event_functions exit_syscall_print_funcs;
>
> #define SYSCALL_TRACE_ENTER_EVENT(sname) \
> - static struct syscall_metadata __syscall_meta_##sname; \
> + static struct syscall_metadata \
> + __attribute__((__aligned__(4))) __syscall_meta_##sname; \
> static struct ftrace_event_call \
> __attribute__((__aligned__(4))) event_enter_##sname; \
> static struct ftrace_event_call __used \
> @@ -138,7 +139,8 @@ extern struct trace_event_functions exit_syscall_print_funcs;
> }
>
> #define SYSCALL_TRACE_EXIT_EVENT(sname) \
> - static struct syscall_metadata __syscall_meta_##sname; \
> + static struct syscall_metadata \
> + __attribute__((__aligned__(4))) __syscall_meta_##sname; \
> static struct ftrace_event_call \
> __attribute__((__aligned__(4))) event_exit_##sname; \
> static struct ftrace_event_call __used \

This looks like a fix that just hide the real bug.
If I remember the original report correct the problem is
that the symbol:

__start_syscalls_metadata

Does not point to a valid syscall entry.

The symbol is assigned in vmlinux.lds.h like this:
#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
*(__syscalls_metadata) \
VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;

Now consider what is happening if we have the following scanario:

. equals 0x1004 so __start_syscalls_metadata is set to 0x1004
But __syscall_metadata require 8 byte alignment so it starts at 0x1008.

Then __start_syscalls_metadata fails to point at the first entry and
this code will fail:

start = (struct syscall_metadata *)__start_syscalls_metadata;
for ( ; start < stop; start++) {
if (start->name && !strcmp(start->name + 3, str + 3))
return start;

iThe right fix seems to me that we guarantee that __start_syscalls_metadata
is assinged a proper address.

Something like this:
(whitespace damaged)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 48c5299..64430d3 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -133,7 +133,8 @@
#endif

#ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
+#define TRACE_SYSCALLS() . = ALIGN(8); \
+ VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
*(__syscalls_metadata) \
VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
#else

But at the same time we should audit the other trace related symbols
in vmlinux.lds.h to see if they may suffer from the same potential bug.

__start_ftrace_events looks like it may have the same potential bug..

The reason why your patch works is that you force a smaller alignment
and this happens by pure luck to be the alignmnet of "." from the
previous section.

Sam

2010-07-09 20:46:18

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, 2010-07-09 at 22:33 +0200, Sam Ravnborg wrote:
> On Fri, Jul 09, 2010 at 03:56:42PM -0400, Steven Rostedt wrote:

> This looks like a fix that just hide the real bug.
> If I remember the original report correct the problem is
> that the symbol:
>
> __start_syscalls_metadata
>
> Does not point to a valid syscall entry.
>
> The symbol is assigned in vmlinux.lds.h like this:
> #define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
> *(__syscalls_metadata) \
> VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
>
> Now consider what is happening if we have the following scanario:
>
> . equals 0x1004 so __start_syscalls_metadata is set to 0x1004
> But __syscall_metadata require 8 byte alignment so it starts at 0x1008.

I did not know that the linker could start a section at a half a word
size. That seems to me to be a linker bug.

If a word for a box is 8 bytes than the linker had better start sections
on 8 byte boundaries. Otherwise I would think other things may break.

For 4 byte word boxes, this should be safe anyway.

-- Steve

2010-07-09 20:53:22

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, Jul 09, 2010 at 04:46:14PM -0400, Steven Rostedt wrote:
> On Fri, 2010-07-09 at 22:33 +0200, Sam Ravnborg wrote:
> > On Fri, Jul 09, 2010 at 03:56:42PM -0400, Steven Rostedt wrote:
>
> > This looks like a fix that just hide the real bug.
> > If I remember the original report correct the problem is
> > that the symbol:
> >
> > __start_syscalls_metadata
> >
> > Does not point to a valid syscall entry.
> >
> > The symbol is assigned in vmlinux.lds.h like this:
> > #define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
> > *(__syscalls_metadata) \
> > VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
> >
> > Now consider what is happening if we have the following scanario:
> >
> > . equals 0x1004 so __start_syscalls_metadata is set to 0x1004
> > But __syscall_metadata require 8 byte alignment so it starts at 0x1008.
>
>
> I did not know that the linker could start a section at a half a word
> size. That seems to me to be a linker bug.

In this case the linker does not start a section - we are in the
middle of an output section.

*(__syscalls_metadata)

Is only used to tell the linker that it shall include the content
of the "__syscalls_metadata" input section in the current output section.

So what we have here is something like this:

.data : AT(ADDR(.data) - LOAD_OFFSET) {
*(_ftrace_events)
__start_syscalls_metadata = .;
*(__syscalls_metadata)
}

.data is the outpud section - and the linker will align the output section
to the biggest alignmnet it see within the referenced input sections.

But in this case we have no control of the value of "." (current address)
when we have processed (_ftrace_events) so it may even be at a 2 byte boundary.
The linker will add padding as needed to satisfy the alignmnet of
__syscalls_metadata - but that padding will be inbetween "." and the first
member in __syscalls_metadata.

Sam

2010-07-09 21:05:54

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, 2010-07-09 at 22:53 +0200, Sam Ravnborg wrote:

> But in this case we have no control of the value of "." (current address)
> when we have processed (_ftrace_events) so it may even be at a 2 byte boundary.
> The linker will add padding as needed to satisfy the alignmnet of
> __syscalls_metadata - but that padding will be inbetween "." and the first
> member in __syscalls_metadata.

Fine, but this is a separate issue. I doubt the "ALIGN(8);" would have
helped us anyway. Remember what the issue we had:

ffffffff8173c438 <__start_syscalls_metadata>:
...

ffffffff8173c440 <__syscall_meta__mmap>:

__start_syscalls_metadata was already aligned to 8, but for some strange
reason, gcc decided to align the first member to 16.

-- Steve

2010-07-09 21:26:29

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, Jul 9, 2010 at 1:33 PM, Sam Ravnborg <[email protected]> wrote:
>
> Something like this:
> (whitespace damaged)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 48c5299..64430d3 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -133,7 +133,8 @@
> ?#endif
>
> ?#ifdef CONFIG_FTRACE_SYSCALLS
> -#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; ? ? ? ?\
> +#define TRACE_SYSCALLS() . = ALIGN(8); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? ? ? ? ? ? ? ? ? ?VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
> ? ? ? ? ? ? ? ? ? ? ? ? *(__syscalls_metadata) ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ? ? ? ? ? ? ? ? ? VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
> ?#else

If this is confirmed to fix it, then I would much prefer this version.

That said, I do wonder whether we shouldn't do the ALIGN(8) in the
DATA_DATA define instead. That's what we do for other things like this
(start_markers, start__verbose, etc etc)

Linus

2010-07-09 21:56:06

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, Jul 09, 2010 at 05:05:50PM -0400, Steven Rostedt wrote:
> On Fri, 2010-07-09 at 22:53 +0200, Sam Ravnborg wrote:
>
> > But in this case we have no control of the value of "." (current address)
> > when we have processed (_ftrace_events) so it may even be at a 2 byte boundary.
> > The linker will add padding as needed to satisfy the alignmnet of
> > __syscalls_metadata - but that padding will be inbetween "." and the first
> > member in __syscalls_metadata.
>
> Fine, but this is a separate issue.
No - this is the exact issue we are facing here.

In the linker script we have no control what-so-ever on the alignment of "."
so relying on any alignment is a potential bug.

In this particular case gcc decide to increase the alignment of
syscall_metadata from 8 to 16.

And then suddenly things fall apart because we rely on "." being
properly aligned - and it is only 4 byte aligned.

So you fixed this by forcing gcc to use a smaller alignment
for syscall_metadata (from 16 => 4).
But what happens next time when "." is two-byte aligned?
We will not change aligment down to 2...

So to fix this properly we need to make sure that "." is properly
aligned to the alignment of syscall_metadata.

In other words - the linker script needs to be fixed too.
But the fix in the .h file is required so we know the alignmnet
of syscall_metadata.

Sam

2010-07-09 22:02:55

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, Jul 09, 2010 at 05:05:50PM -0400, Steven Rostedt wrote:
> On Fri, 2010-07-09 at 22:53 +0200, Sam Ravnborg wrote:
>
> > But in this case we have no control of the value of "." (current address)
> > when we have processed (_ftrace_events) so it may even be at a 2 byte boundary.
> > The linker will add padding as needed to satisfy the alignmnet of
> > __syscalls_metadata - but that padding will be inbetween "." and the first
> > member in __syscalls_metadata.
>
> Fine, but this is a separate issue. I doubt the "ALIGN(8);" would have
> helped us anyway. Remember what the issue we had:
>
> ffffffff8173c438 <__start_syscalls_metadata>:
> ...
>
> ffffffff8173c440 <__syscall_meta__mmap>:
>
>
> __start_syscalls_metadata was already aligned to 8, but for some strange
> reason, gcc decided to align the first member to 16.

I found some more info in following bug:
https://bugzillafiles.novell.org/attachment.cgi?id=344563

Jeff Mahoney says:
GCC 4.5 introduced behavior that forces the alignment of structures to
use the largest possible value. The default value is 32 bytes, so if
some structures are defined with a 4-byte alignment and others aren't
declared with an alignment constraint at all - it will align at 32-bytes.

So according to this we can rely on 32 byte alignment and
this is trivial to add in vmlinux.lds.h.

Sam

2010-07-10 00:22:24

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Fri, 2010-07-09 at 14:25 -0700, Linus Torvalds wrote:
> On Fri, Jul 9, 2010 at 1:33 PM, Sam Ravnborg <[email protected]> wrote:
> >
> > Something like this:
> > (whitespace damaged)
> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index 48c5299..64430d3 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -133,7 +133,8 @@
> > #endif
> >
> > #ifdef CONFIG_FTRACE_SYSCALLS
> > -#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
> > +#define TRACE_SYSCALLS() . = ALIGN(8); \
> > + VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
> > *(__syscalls_metadata) \
> > VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
> > #else
>
> If this is confirmed to fix it, then I would much prefer this version.

Zeev,

Can you try Sam's version and remove mine. I'd like to see if that fixes
the issue too.

Thanks,

-- Steve

2010-07-10 06:35:05

[permalink] [raw]

Subject: [PATCH] tracing: properly align linker defined symbols

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 <[email protected]>
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 <[email protected]>
Signed-off-by: Sam Ravnborg <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
---
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

2010-07-10 09:45:51

[permalink] [raw]

Subject: Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment to syscall metadata declarations

On Sat, Jul 10, 2010 at 03:22, Steven Rostedt <[email protected]> wrote:
> On Fri, 2010-07-09 at 14:25 -0700, Linus Torvalds wrote:
>> On Fri, Jul 9, 2010 at 1:33 PM, Sam Ravnborg <[email protected]> wrote:
>> >
>> > Something like this:
>> > (whitespace damaged)
>> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> > index 48c5299..64430d3 100644
>> > --- a/include/asm-generic/vmlinux.lds.h
>> > +++ b/include/asm-generic/vmlinux.lds.h
>> > @@ -133,7 +133,8 @@
>> > ?#endif
>> >
>> > ?#ifdef CONFIG_FTRACE_SYSCALLS
>> > -#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; ? ? ? ?\
>> > +#define TRACE_SYSCALLS() . = ALIGN(8); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> > + ? ? ? ? ? ? ? ? ? ? ? ?VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
>> > ? ? ? ? ? ? ? ? ? ? ? ? *(__syscalls_metadata) ? ? ? ? ? ? ? ? ? ? ? ? \
>> > ? ? ? ? ? ? ? ? ? ? ? ? VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
>> > ?#else
>>
>> If this is confirmed to fix it, then I would much prefer this version.
>
> Zeev,
>
> Can you try Sam's version and remove mine. I'd like to see if that fixes
> the issue too.
>
> Thanks,
>
> -- Steve
>
>
>

Clean kernel source from tarball with only this patch applied, with
copied over config file, compiled with my gcc 4.5.1 doesn't boot. Same
call stack.
The disassembly begins same way as for unpatched source (0x...38 , 0x...40).
Now trying the patch to "Align to a 32 byte boundary".

-Zeev

2010-07-10 10:19:16

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Sam Ravnborg <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> ---
> ?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 <[email protected]>

-Zeev

2010-07-10 11:34:11

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote:
> On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <[email protected]> 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

2010-07-10 13:06:36

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Sat, Jul 10, 2010 at 14:34, Steven Rostedt <[email protected]> wrote:
> On Sat, 2010-07-10 at 13:18 +0300, Zeev Tarantov wrote:
>> On Sat, Jul 10, 2010 at 09:35, Sam Ravnborg <[email protected]> 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

2010-07-10 13:48:53

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

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 <[email protected]> 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;
}

2010-07-10 13:49:41

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

>
> 2.6.35-rc4 from tarball patched with only this, same config & same
> compiler boots fine.
>
> Tested-by: Zeev Tarantov <[email protected]>

Thanks for your paitient testing Zeev!

Sam

2010-07-10 15:36:42

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Sat, Jul 10, 2010 at 16:48, Sam Ravnborg <[email protected]> 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 <[email protected]> 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 <[email protected]>

-Zeev

2010-07-10 22:25:41

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Fri, Jul 9, 2010 at 11:35 PM, Sam Ravnborg <[email protected]> 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 <[email protected]>
> Signed-off-by: Sam Ravnborg <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> ---
> ?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
>
>

Attachments:

diff (3.27 kB)

2010-07-10 22:28:02

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Sat, Jul 10, 2010 at 3:25 PM, Linus Torvalds
<[email protected]> 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

2010-07-11 06:07:28

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

On Sat, Jul 10, 2010 at 03:25:02PM -0700, Linus Torvalds wrote:
> On Fri, Jul 9, 2010 at 11:35 PM, Sam Ravnborg <[email protected]> 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

2010-07-15 15:05:23

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Sam Ravnborg <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> ---
> 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
>

2010-07-15 15:16:13

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

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

2010-07-15 18:31:13

[permalink] [raw]

Subject: Re: [PATCH] tracing: properly align linker defined symbols

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

2010-07-22 11:52:09