| Message ID | 20110121203642.725191236@efficios.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Date: Fri, 21 Jan 2011 15:36:31 -0500 > Problem description: > > gcc happily align on 32-byte structures defined statically. Ftrace trace events > and Tracepoints both statically define structures into custom sections (using > the "section" attribute), to then assign these to symbols with the linker > scripts to iterate the these sections as an array. > > However, gcc uses different alignments for these structures when they are > defined statically than when they are globally visible and/or in an array. > Therefore iteration on these arrays sees "holes" of padding. gcc is within its > rights to increase the alignment of the statically defined structures because, > normally, there should be no other accesses to them than in the local object. We > are actually iterating on the generated structures as if they were an array > without letting gcc knowing anything about it. > > This patch introduces __u64_aligned to force gcc to use the u64 type and > variable alignment, up-aligning or down-aligning the target type if necessary. > The memory accesses to the target structure are efficient (does not require > bytewise memory accesses) and the atomic pointer update guarantees required by > RCU are kept. u64 is considered as the largest type that can generate a trap for > unaligned accesses (u64 on sparc32 needs to be aligned on 64-bit). > > This alignment should be used for both structure definitions and declarations > (as *both* the type and variable attribute) when using the "section" > attribute to generate arrays of structures. Given that gcc only uses the type > attribute "aligned" as a lower-bound for alignment, the structures should not > contain types which require alignment larger than that of u64. The "aligned" > variable attribute, on the other hand, forces gcc to use exactly the specified > alignment. > > Also introduce the linker script U64_ALIGN() macro for specification of custom > section alignment that matches that of __u64_aligned. > > Changelog since v2: > - Drop the "packed" type attribute, because it causes gcc to drop the padding > between consecutive "int" and "pointer"/"long" fields, which leads to > unaligned accesses on sparc64. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Acked-by: David S. Miller <davem@davemloft.net> -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* David Miller (davem@davemloft.net) wrote: > From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Date: Fri, 21 Jan 2011 15:36:31 -0500 > > > Problem description: > > > > gcc happily align on 32-byte structures defined statically. Ftrace trace events > > and Tracepoints both statically define structures into custom sections (using > > the "section" attribute), to then assign these to symbols with the linker > > scripts to iterate the these sections as an array. > > > > However, gcc uses different alignments for these structures when they are > > defined statically than when they are globally visible and/or in an array. > > Therefore iteration on these arrays sees "holes" of padding. gcc is within its > > rights to increase the alignment of the statically defined structures because, > > normally, there should be no other accesses to them than in the local object. We > > are actually iterating on the generated structures as if they were an array > > without letting gcc knowing anything about it. > > > > This patch introduces __u64_aligned to force gcc to use the u64 type and > > variable alignment, up-aligning or down-aligning the target type if necessary. > > The memory accesses to the target structure are efficient (does not require > > bytewise memory accesses) and the atomic pointer update guarantees required by > > RCU are kept. u64 is considered as the largest type that can generate a trap for > > unaligned accesses (u64 on sparc32 needs to be aligned on 64-bit). > > > > This alignment should be used for both structure definitions and declarations > > (as *both* the type and variable attribute) when using the "section" > > attribute to generate arrays of structures. Given that gcc only uses the type > > attribute "aligned" as a lower-bound for alignment, the structures should not > > contain types which require alignment larger than that of u64. The "aligned" > > variable attribute, on the other hand, forces gcc to use exactly the specified > > alignment. > > > > Also introduce the linker script U64_ALIGN() macro for specification of custom > > section alignment that matches that of __u64_aligned. > > > > Changelog since v2: > > - Drop the "packed" type attribute, because it causes gcc to drop the padding > > between consecutive "int" and "pointer"/"long" fields, which leads to > > unaligned accesses on sparc64. > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > Acked-by: David S. Miller <davem@davemloft.net> Thanks David, After these patches get merged -- I would recommend going through the tracing tree -- they'll have to go to -stable too. I'm therefore forwarding this to stable@kernel.org (the same should be done for the two other patches in this series). Mathieu
On Sat, 2011-01-22 at 12:11 -0500, Mathieu Desnoyers wrote: > After these patches get merged -- I would recommend going through the tracing > tree -- they'll have to go to -stable too. I'm therefore forwarding this to > stable@kernel.org (the same should be done for the two other patches in this > series). Hi Mathieu, I was in the process of putting them through my ktest.pl patch-check tests, but unfortunately, another bug was triggered (unrelated to these) and prevented me from getting them ready last night. I'll continue on them on Monday. -- Steve -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Steven Rostedt (rostedt@goodmis.org) wrote: > On Sat, 2011-01-22 at 12:11 -0500, Mathieu Desnoyers wrote: > > > After these patches get merged -- I would recommend going through the tracing > > tree -- they'll have to go to -stable too. I'm therefore forwarding this to > > stable@kernel.org (the same should be done for the two other patches in this > > series). > > Hi Mathieu, > > I was in the process of putting them through my ktest.pl patch-check > tests, but unfortunately, another bug was triggered (unrelated to these) > and prevented me from getting them ready last night. > > I'll continue on them on Monday. Hi Steven, Any progress on the merge process for this bugfix patch series ? The issue seemed rather pressing last week, given that it breaks sparc64. Thanks, Mathieu
On Tue, 2011-01-25 at 18:34 -0500, Mathieu Desnoyers wrote: > Any progress on the merge process for this bugfix patch series ? The issue > seemed rather pressing last week, given that it breaks sparc64. I was hoping to resolve the bug, but its not that reproducible and screws up the bisect. I'll disable the feature that is broken (for now) and continue with your patches. Hopefully everything will go smoothly, and I can push these out tonight or tomorrow morning. -- Steve -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6-lttng/include/linux/compiler.h =================================================================== --- linux-2.6-lttng.orig/include/linux/compiler.h +++ linux-2.6-lttng/include/linux/compiler.h @@ -57,6 +57,8 @@ extern void __chk_io_ptr(const volatile # include <linux/compiler-intel.h> #endif +#include <linux/align-section.h> + /* * Generic compiler-dependent macros required for kernel * build go below this comment. Actual compiler/compiler version Index: linux-2.6-lttng/include/asm-generic/vmlinux.lds.h =================================================================== --- linux-2.6-lttng.orig/include/asm-generic/vmlinux.lds.h +++ linux-2.6-lttng/include/asm-generic/vmlinux.lds.h @@ -69,6 +69,12 @@ */ #define STRUCT_ALIGN() . = ALIGN(32) +/* + * Align to a 8 byte boundary. For use with custom section made from structures + * declared and defined with __u64_aligned. + */ +#define U64_ALIGN() . = ALIGN(8) + /* The actual configuration determine if the init/exit sections * are handled as text/data or they can be discarded (which * often happens at runtime) Index: linux-2.6-lttng/include/linux/align-section.h =================================================================== --- /dev/null +++ linux-2.6-lttng/include/linux/align-section.h @@ -0,0 +1,54 @@ +#ifndef _LINUX_ALIGN_SECTION_H +#define _LINUX_ALIGN_SECTION_H + +/* + * __u64_aligned: + * + * __u64_aligned should be used as type and variable attribute for structure + * definitions when using the "section" attribute to generate arrays of + * structures. U64_ALIGN() must be used prior to these section definitions in + * the linker script. + * + * It forces the compiler to use the u64 type alignment, up-aligning or + * down-aligning the target type if necessary. The memory accesses to the target + * structure are efficient (does not require bytewise memory accesses) and the + * atomic pointer update guarantees required by RCU are kept. u64 is considered + * as the largest type that can generate a trap for unaligned accesses (u64 on + * sparc32 needs to be aligned on 64-bit). + * + * Given that gcc only uses the type attribute "aligned" as a lower-bound for + * alignment, the structures should not contain types which require alignment + * larger than that of u64. The "aligned" variable attribute, on the other hand, + * forces gcc to use exactly the specified alignment. + */ + +/* + * Use __u64_aligned as type and variable attribute for custom section structure + * declaration and definition. It should also be applied to any static or + * extern definition of the structure that would override the definition to + * which the "section" attribute is applied, e.g. + * + * struct custom { + * unsigned long field; + * ... + * } __u64_aligned; + * + * extern struct __u64_aligned custom; + * struct custom __u64_aligned __attribute__((section("__custom")) identifier; + * + * The array can then be defined with: + * + * extern struct custom __start___custom[]; + * extern struct custom __stop___custom[]; + * + * With linking performed by the linker script: + * + * U64_ALIGN(); + * VMLINUX_SYMBOL(__start___custom) = .; + * *(__custom) + * VMLINUX_SYMBOL(__stop___custom) = .; + */ + +#define __u64_aligned __attribute__((__aligned__(__alignof__(long long)))) + +#endif /* _LINUX_ALIGN_SECTION_H */
Problem description: gcc happily align on 32-byte structures defined statically. Ftrace trace events and Tracepoints both statically define structures into custom sections (using the "section" attribute), to then assign these to symbols with the linker scripts to iterate the these sections as an array. However, gcc uses different alignments for these structures when they are defined statically than when they are globally visible and/or in an array. Therefore iteration on these arrays sees "holes" of padding. gcc is within its rights to increase the alignment of the statically defined structures because, normally, there should be no other accesses to them than in the local object. We are actually iterating on the generated structures as if they were an array without letting gcc knowing anything about it. This patch introduces __u64_aligned to force gcc to use the u64 type and variable alignment, up-aligning or down-aligning the target type if necessary. The memory accesses to the target structure are efficient (does not require bytewise memory accesses) and the atomic pointer update guarantees required by RCU are kept. u64 is considered as the largest type that can generate a trap for unaligned accesses (u64 on sparc32 needs to be aligned on 64-bit). This alignment should be used for both structure definitions and declarations (as *both* the type and variable attribute) when using the "section" attribute to generate arrays of structures. Given that gcc only uses the type attribute "aligned" as a lower-bound for alignment, the structures should not contain types which require alignment larger than that of u64. The "aligned" variable attribute, on the other hand, forces gcc to use exactly the specified alignment. Also introduce the linker script U64_ALIGN() macro for specification of custom section alignment that matches that of __u64_aligned. Changelog since v2: - Drop the "packed" type attribute, because it causes gcc to drop the padding between consecutive "int" and "pointer"/"long" fields, which leads to unaligned accesses on sparc64. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> CC: David Miller <davem@davemloft.net> CC: Steven Rostedt <rostedt@goodmis.org> CC: Frederic Weisbecker <fweisbec@gmail.com> CC: Ingo Molnar <mingo@elte.hu> --- include/asm-generic/vmlinux.lds.h | 6 ++++ include/linux/align-section.h | 54 ++++++++++++++++++++++++++++++++++++++ include/linux/compiler.h | 2 + 3 files changed, 62 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html