LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [patch 0/3] Fix alignment of custom sections made from structures (v3)
@ 2011-01-21 20:36 Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3) Mathieu Desnoyers
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-21 20:36 UTC (permalink / raw)
  To: LKML, David Miller, Steven Rostedt, Frederic Weisbecker,
	Ingo Molnar, Richard Mortimer, ben, sparclinux

Hello,

Here is a patchset that fixes the Ftrace-induced crash experienced on sparc64
discussed in the following thread: https://lkml.org/lkml/2011/1/16/2

The third patch changes the Tracepoints to use the more compact alignment scheme
proposed here, even though their alignment on 32-byte works fine. This third
patch is therefore not a fix per se.

The patchset is based on 2.6.37. Hopefully there are not too many things to
update for the current -git.

Changelog since v2: drop the "packed" attribute, because it caused unaligned 
acceses on sparc64 by dropping the padding between consecutive "int" and 
"pointer"/"long" fields.

Comments are welcome,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3)
  2011-01-21 20:36 [patch 0/3] Fix alignment of custom sections made from structures (v3) Mathieu Desnoyers
@ 2011-01-21 20:36 ` Mathieu Desnoyers
  2011-01-22  0:05   ` David Miller
  2011-01-26  7:12   ` [tip:perf/core] Introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections tip-bot for Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN() Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN() Mathieu Desnoyers
  2 siblings, 2 replies; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-21 20:36 UTC (permalink / raw)
  To: LKML, David Miller, Steven Rostedt, Frederic Weisbecker,
	Ingo Molnar, Richard Mortimer, ben, sparclinux
  Cc: Mathieu Desnoyers

[-- Attachment #1: introduce-u64-aligned.patch --]
[-- Type: text/plain, Size: 5866 bytes --]

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(+)

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 */


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN()
  2011-01-21 20:36 [patch 0/3] Fix alignment of custom sections made from structures (v3) Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3) Mathieu Desnoyers
@ 2011-01-21 20:36 ` Mathieu Desnoyers
  2011-01-22  0:05   ` David Miller
  2011-01-26  7:13   ` [tip:perf/core] tracing: Fix " tip-bot for Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN() Mathieu Desnoyers
  2 siblings, 2 replies; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-21 20:36 UTC (permalink / raw)
  To: LKML, David Miller, Steven Rostedt, Frederic Weisbecker,
	Ingo Molnar, Richard Mortimer, ben, sparclinux
  Cc: Mathieu Desnoyers

[-- Attachment #1: tracing-use-u64-aligned-as-type-and-variable-attribute.patch --]
[-- Type: text/plain, Size: 11516 bytes --]

Problem description:

gcc happily align structures defined statically on 32-byte. Ftrace trace events
and Tracepoints both statically define structures into 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 and when they are globally visible and/or in an array.
Therefore iteration on these arrays sees "holes" of padding.

Use the __u64_aligned for type declarations and variable definitions to ensure
that gcc:

a) iterates on the correctly aligned type. (type attribute)
b) generates the definitions within the sections with the appropriate alignment.
   (variable attribute)

The Ftrace code introduced the "aligned(4)" variable attribute in commit
1473e4417c79f12d91ef91a469699bfa911f510f to try to work around this problem that
showed up on x86_64, but it causes unaligned accesses on sparc64, and is
generally a bad idea on 64-bit if RCU pointers are contained within the
structure. Moreover, it did not use the same attribute as type attribute, which
could cause the iteration on the extern array structure not to match the
variable definitions for some structure sizes.

We should also ensure proper alignment of each Ftrace section in
include/asm-generic/vmlinux.lds.h.

Moving all STRUCT_ALIGN() for FTRACE_EVENTS() and TRACE_SYSCALLS() into the
definitions, so the alignment is only done if these infrastructures are
configured in. Use U64_ALIGN instead of STRUCT_ALIGN.

Also align TRACE_PRINTKS on U64_ALIGN to make sure the beginning of the section
is aligned on pointer size.

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 |   19 ++++++++++---------
 include/linux/compiler.h          |    6 +++---
 include/linux/ftrace_event.h      |    2 +-
 include/linux/syscalls.h          |   18 ++++++++----------
 include/trace/ftrace.h            |    8 ++++----
 include/trace/syscall.h           |    2 +-
 kernel/trace/trace.h              |    2 +-
 kernel/trace/trace_export.c       |    2 +-
 8 files changed, 29 insertions(+), 30 deletions(-)

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
@@ -80,7 +80,7 @@ struct ftrace_branch_data {
 		};
 		unsigned long miss_hit[2];
 	};
-};
+} __u64_aligned;
 
 /*
  * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code
@@ -96,7 +96,7 @@ void ftrace_likely_update(struct ftrace_
 #define __branch_check__(x, expect) ({					\
 			int ______r;					\
 			static struct ftrace_branch_data		\
-				__attribute__((__aligned__(4)))		\
+				__u64_aligned				\
 				__attribute__((section("_ftrace_annotated_branch"))) \
 				______f = {				\
 				.func = __func__,			\
@@ -131,7 +131,7 @@ void ftrace_likely_update(struct ftrace_
 	({								\
 		int ______r;						\
 		static struct ftrace_branch_data			\
-			__attribute__((__aligned__(4)))			\
+			__u64_aligned					\
 			__attribute__((section("_ftrace_branch")))	\
 			______f = {					\
 				.func = __func__,			\
Index: linux-2.6-lttng/include/linux/syscalls.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/syscalls.h
+++ linux-2.6-lttng/include/linux/syscalls.h
@@ -126,12 +126,11 @@ extern struct trace_event_functions exit
 
 #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
 	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	  __u64_aligned __syscall_meta_##sname;				\
 	static struct ftrace_event_call					\
-	__attribute__((__aligned__(4))) event_enter_##sname;		\
+	  __u64_aligned event_enter_##sname;				\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
-	  __attribute__((section("_ftrace_events")))			\
+	  __u64_aligned __attribute__((section("_ftrace_events")))	\
 	  event_enter_##sname = {					\
 		.name                   = "sys_enter"#sname,		\
 		.class			= &event_class_syscall_enter,	\
@@ -141,12 +140,11 @@ extern struct trace_event_functions exit
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
 	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	__u64_aligned __syscall_meta_##sname;				\
 	static struct ftrace_event_call					\
-	__attribute__((__aligned__(4))) event_exit_##sname;		\
+	__u64_aligned event_exit_##sname;				\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
-	  __attribute__((section("_ftrace_events")))			\
+	  __u64_aligned __attribute__((section("_ftrace_events")))	\
 	  event_exit_##sname = {					\
 		.name                   = "sys_exit"#sname,		\
 		.class			= &event_class_syscall_exit,	\
@@ -158,7 +156,7 @@ extern struct trace_event_functions exit
 	SYSCALL_TRACE_ENTER_EVENT(sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
+	  __u64_aligned						\
 	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta_##sname = {				\
 		.name 		= "sys"#sname,			\
@@ -174,7 +172,7 @@ extern struct trace_event_functions exit
 	SYSCALL_TRACE_ENTER_EVENT(_##sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(_##sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
+	  __u64_aligned						\
 	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta__##sname = {				\
 		.name 		= "sys_"#sname,			\
Index: linux-2.6-lttng/include/trace/ftrace.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/ftrace.h
+++ linux-2.6-lttng/include/trace/ftrace.h
@@ -69,7 +69,7 @@
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, name, proto, args)	\
 	static struct ftrace_event_call	__used		\
-	__attribute__((__aligned__(4))) event_##name
+	__u64_aligned event_##name;
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
@@ -434,7 +434,7 @@ static inline notrace int ftrace_get_off
  * };
  *
  * static struct ftrace_event_call __used
- * __attribute__((__aligned__(4)))
+ * __u64_aligned
  * __attribute__((section("_ftrace_events"))) event_<call> = {
  *	.name			= "<call>",
  *	.class			= event_class_<template>,
@@ -567,7 +567,7 @@ static struct ftrace_event_class __used
 #define DEFINE_EVENT(template, call, proto, args)			\
 									\
 static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
@@ -581,7 +581,7 @@ __attribute__((section("_ftrace_events")
 static const char print_fmt_##call[] = print;				\
 									\
 static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
Index: linux-2.6-lttng/kernel/trace/trace.h
===================================================================
--- linux-2.6-lttng.orig/kernel/trace/trace.h
+++ linux-2.6-lttng/kernel/trace/trace.h
@@ -749,7 +749,7 @@ extern const char *__stop___trace_bprint
 #undef FTRACE_ENTRY
 #define FTRACE_ENTRY(call, struct_name, id, tstruct, print)		\
 	extern struct ftrace_event_call					\
-	__attribute__((__aligned__(4))) event_##call;
+	__u64_aligned event_##call;
 #undef FTRACE_ENTRY_DUP
 #define FTRACE_ENTRY_DUP(call, struct_name, id, tstruct, print)		\
 	FTRACE_ENTRY(call, struct_name, id, PARAMS(tstruct), PARAMS(print))
Index: linux-2.6-lttng/kernel/trace/trace_export.c
===================================================================
--- linux-2.6-lttng.orig/kernel/trace/trace_export.c
+++ linux-2.6-lttng/kernel/trace/trace_export.c
@@ -156,7 +156,7 @@ struct ftrace_event_class event_class_ft
 };									\
 									\
 struct ftrace_event_call __used						\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.event.type		= etype,				\
Index: linux-2.6-lttng/include/linux/ftrace_event.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/ftrace_event.h
+++ linux-2.6-lttng/include/linux/ftrace_event.h
@@ -194,7 +194,7 @@ struct ftrace_event_call {
 	int				perf_refcount;
 	struct hlist_head __percpu	*perf_events;
 #endif
-};
+} __u64_aligned;
 
 #define PERF_MAX_TRACE_SIZE	2048
 
Index: linux-2.6-lttng/include/trace/syscall.h
===================================================================
--- linux-2.6-lttng.orig/include/trace/syscall.h
+++ linux-2.6-lttng/include/trace/syscall.h
@@ -29,7 +29,7 @@ struct syscall_metadata {
 
 	struct ftrace_event_call *enter_event;
 	struct ftrace_event_call *exit_event;
-};
+} __u64_aligned;
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 extern unsigned long arch_syscall_addr(int nr);
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
@@ -113,7 +113,8 @@
 #endif
 
 #ifdef CONFIG_TRACE_BRANCH_PROFILING
-#define LIKELY_PROFILE()	VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
+#define LIKELY_PROFILE()	U64_ALIGN();					      \
+				VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
 				*(_ftrace_annotated_branch)			      \
 				VMLINUX_SYMBOL(__stop_annotated_branch_profile) = .;
 #else
@@ -121,7 +122,8 @@
 #endif
 
 #ifdef CONFIG_PROFILE_ALL_BRANCHES
-#define BRANCH_PROFILE()	VMLINUX_SYMBOL(__start_branch_profile) = .;   \
+#define BRANCH_PROFILE()	U64_ALIGN();				      \
+				VMLINUX_SYMBOL(__start_branch_profile) = .;   \
 				*(_ftrace_branch)			      \
 				VMLINUX_SYMBOL(__stop_branch_profile) = .;
 #else
@@ -129,7 +131,8 @@
 #endif
 
 #ifdef CONFIG_EVENT_TRACING
-#define FTRACE_EVENTS()	VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
+#define FTRACE_EVENTS()	U64_ALIGN();					\
+			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
 			*(_ftrace_events)				\
 			VMLINUX_SYMBOL(__stop_ftrace_events) = .;
 #else
@@ -137,7 +140,8 @@
 #endif
 
 #ifdef CONFIG_TRACING
-#define TRACE_PRINTKS() VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .;      \
+#define TRACE_PRINTKS()  U64_ALIGN();					       \
+			 VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .;      \
 			 *(__trace_printk_fmt) /* Trace_printk fmt' pointer */ \
 			 VMLINUX_SYMBOL(__stop___trace_bprintk_fmt) = .;
 #else
@@ -145,7 +149,8 @@
 #endif
 
 #ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
+#define TRACE_SYSCALLS() U64_ALIGN();					\
+			 VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
 			 *(__syscalls_metadata)				\
 			 VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
 #else
@@ -175,11 +180,7 @@
 	LIKELY_PROFILE()		       				\
 	BRANCH_PROFILE()						\
 	TRACE_PRINTKS()							\
-									\
-	STRUCT_ALIGN();							\
 	FTRACE_EVENTS()							\
-									\
-	STRUCT_ALIGN();							\
 	TRACE_SYSCALLS()
 
 /*


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN()
  2011-01-21 20:36 [patch 0/3] Fix alignment of custom sections made from structures (v3) Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3) Mathieu Desnoyers
  2011-01-21 20:36 ` [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN() Mathieu Desnoyers
@ 2011-01-21 20:36 ` Mathieu Desnoyers
  2011-01-22  0:05   ` David Miller
  2011-01-26  7:13   ` [tip:perf/core] tracepoints: Use __u64_aligned/U64_ALIGN() tip-bot for Mathieu Desnoyers
  2 siblings, 2 replies; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-21 20:36 UTC (permalink / raw)
  To: LKML, David Miller, Steven Rostedt, Frederic Weisbecker,
	Ingo Molnar, Richard Mortimer, ben, sparclinux
  Cc: Mathieu Desnoyers

[-- Attachment #1: tracepoints-use-u64-aligned-as-type-and-variable-attribute.patch --]
[-- Type: text/plain, Size: 3335 bytes --]

commit 7e066fb870fcd1025ec3ba7bbde5d541094f4ce1 added the aligned(32) type and
variable attribute to the tracepoint structures to deal with gcc happily
aligning statically defined structures on 32-byte multiples.

Working on issues within Ftrace, we came up with __64_aligned, which deals with
this issue more elegantly by forcing an 8-byte alignment to both the type
declaration and variable definition.

This therefore saves space, bringing down the size of struct tracepoint from 64
bytes to 38 on 64-bit architectures.

Updating:
- The type attribute (for iteration over the struct tracepoint array)
- Added the variable attribute to the extern definition (needed to force gcc to
  consider this alignment for the following definition)
- The definition variable attribute (to force gcc to this specific alignment for
  the static definitions)
- The linker script (8-byte alignment can now replace the previous 32-byte
  alignment for the custom tracepoint section)

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 |    2 +-
 include/linux/tracepoint.h        |   12 ++++--------
 2 files changed, 5 insertions(+), 9 deletions(-)

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
@@ -168,7 +168,7 @@
 	CPU_KEEP(exit.data)						\
 	MEM_KEEP(init.data)						\
 	MEM_KEEP(exit.data)						\
-	. = ALIGN(32);							\
+	U64_ALIGN();							\
 	VMLINUX_SYMBOL(__start___tracepoints) = .;			\
 	*(__tracepoints)						\
 	VMLINUX_SYMBOL(__stop___tracepoints) = .;			\
Index: linux-2.6-lttng/include/linux/tracepoint.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/tracepoint.h
+++ linux-2.6-lttng/include/linux/tracepoint.h
@@ -33,12 +33,7 @@ struct tracepoint {
 	void (*regfunc)(void);
 	void (*unregfunc)(void);
 	struct tracepoint_func *funcs;
-} __attribute__((aligned(32)));		/*
-					 * Aligned on 32 bytes because it is
-					 * globally visible and gcc happily
-					 * align these on the structure size.
-					 * Keep in sync with vmlinux.lds.h.
-					 */
+} __u64_aligned;
 
 /*
  * Connect a probe to a tracepoint.
@@ -143,7 +138,7 @@ static inline void tracepoint_update_pro
  * structure. Force alignment to the same alignment as the section start.
  */
 #define __DECLARE_TRACE(name, proto, args, data_proto, data_args)	\
-	extern struct tracepoint __tracepoint_##name;			\
+	extern struct tracepoint __u64_aligned __tracepoint_##name;	\
 	static inline void trace_##name(proto)				\
 	{								\
 		JUMP_LABEL(&__tracepoint_##name.state, do_trace);	\
@@ -174,7 +169,8 @@ do_trace:								\
 	static const char __tpstrtab_##name[]				\
 	__attribute__((section("__tracepoints_strings"))) = #name;	\
 	struct tracepoint __tracepoint_##name				\
-	__attribute__((section("__tracepoints"), aligned(32))) =	\
+	__u64_aligned							\
+	__attribute__((section("__tracepoints"))) =			\
 		{ __tpstrtab_##name, 0, reg, unreg, NULL }
 
 #define DEFINE_TRACE(name)						\


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3)
  2011-01-21 20:36 ` [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3) Mathieu Desnoyers
@ 2011-01-22  0:05   ` David Miller
  2011-01-22 17:11     ` Mathieu Desnoyers
  2011-01-26  7:12   ` [tip:perf/core] Introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections tip-bot for Mathieu Desnoyers
  1 sibling, 1 reply; 16+ messages in thread
From: David Miller @ 2011-01-22  0:05 UTC (permalink / raw)
  To: mathieu.desnoyers
  Cc: linux-kernel, rostedt, fweisbec, mingo, richm, ben, sparclinux

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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN()
  2011-01-21 20:36 ` [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN() Mathieu Desnoyers
@ 2011-01-22  0:05   ` David Miller
  2011-01-22 17:15     ` Mathieu Desnoyers
  2011-01-26  7:13   ` [tip:perf/core] tracing: Fix " tip-bot for Mathieu Desnoyers
  1 sibling, 1 reply; 16+ messages in thread
From: David Miller @ 2011-01-22  0:05 UTC (permalink / raw)
  To: mathieu.desnoyers
  Cc: linux-kernel, rostedt, fweisbec, mingo, richm, ben, sparclinux

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Fri, 21 Jan 2011 15:36:32 -0500

> Problem description:
> 
> gcc happily align structures defined statically on 32-byte. Ftrace trace events
> and Tracepoints both statically define structures into 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 and when they are globally visible and/or in an array.
> Therefore iteration on these arrays sees "holes" of padding.
> 
> Use the __u64_aligned for type declarations and variable definitions to ensure
> that gcc:
> 
> a) iterates on the correctly aligned type. (type attribute)
> b) generates the definitions within the sections with the appropriate alignment.
>    (variable attribute)
> 
> The Ftrace code introduced the "aligned(4)" variable attribute in commit
> 1473e4417c79f12d91ef91a469699bfa911f510f to try to work around this problem that
> showed up on x86_64, but it causes unaligned accesses on sparc64, and is
> generally a bad idea on 64-bit if RCU pointers are contained within the
> structure. Moreover, it did not use the same attribute as type attribute, which
> could cause the iteration on the extern array structure not to match the
> variable definitions for some structure sizes.
> 
> We should also ensure proper alignment of each Ftrace section in
> include/asm-generic/vmlinux.lds.h.
> 
> Moving all STRUCT_ALIGN() for FTRACE_EVENTS() and TRACE_SYSCALLS() into the
> definitions, so the alignment is only done if these infrastructures are
> configured in. Use U64_ALIGN instead of STRUCT_ALIGN.
> 
> Also align TRACE_PRINTKS on U64_ALIGN to make sure the beginning of the section
> is aligned on pointer size.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN()
  2011-01-21 20:36 ` [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN() Mathieu Desnoyers
@ 2011-01-22  0:05   ` David Miller
  2011-01-26  7:13   ` [tip:perf/core] tracepoints: Use __u64_aligned/U64_ALIGN() tip-bot for Mathieu Desnoyers
  1 sibling, 0 replies; 16+ messages in thread
From: David Miller @ 2011-01-22  0:05 UTC (permalink / raw)
  To: mathieu.desnoyers
  Cc: linux-kernel, rostedt, fweisbec, mingo, richm, ben, sparclinux

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Fri, 21 Jan 2011 15:36:33 -0500

> commit 7e066fb870fcd1025ec3ba7bbde5d541094f4ce1 added the aligned(32) type and
> variable attribute to the tracepoint structures to deal with gcc happily
> aligning statically defined structures on 32-byte multiples.
> 
> Working on issues within Ftrace, we came up with __64_aligned, which deals with
> this issue more elegantly by forcing an 8-byte alignment to both the type
> declaration and variable definition.
> 
> This therefore saves space, bringing down the size of struct tracepoint from 64
> bytes to 38 on 64-bit architectures.
> 
> Updating:
> - The type attribute (for iteration over the struct tracepoint array)
> - Added the variable attribute to the extern definition (needed to force gcc to
>   consider this alignment for the following definition)
> - The definition variable attribute (to force gcc to this specific alignment for
>   the static definitions)
> - The linker script (8-byte alignment can now replace the previous 32-byte
>   alignment for the custom tracepoint section)
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3)
  2011-01-22  0:05   ` David Miller
@ 2011-01-22 17:11     ` Mathieu Desnoyers
  2011-01-22 17:43       ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-22 17:11 UTC (permalink / raw)
  To: David Miller
  Cc: linux-kernel, rostedt, fweisbec, mingo, richm, ben, sparclinux, stable

* 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


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN()
  2011-01-22  0:05   ` David Miller
@ 2011-01-22 17:15     ` Mathieu Desnoyers
  0 siblings, 0 replies; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-22 17:15 UTC (permalink / raw)
  To: David Miller
  Cc: linux-kernel, rostedt, fweisbec, mingo, richm, ben, sparclinux, stable

* David Miller (davem@davemloft.net) wrote:
> From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date: Fri, 21 Jan 2011 15:36:32 -0500
> 
> > Problem description:
> > 
> > gcc happily align structures defined statically on 32-byte. Ftrace trace events
> > and Tracepoints both statically define structures into 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 and when they are globally visible and/or in an array.
> > Therefore iteration on these arrays sees "holes" of padding.
> > 
> > Use the __u64_aligned for type declarations and variable definitions to ensure
> > that gcc:
> > 
> > a) iterates on the correctly aligned type. (type attribute)
> > b) generates the definitions within the sections with the appropriate alignment.
> >    (variable attribute)
> > 
> > The Ftrace code introduced the "aligned(4)" variable attribute in commit
> > 1473e4417c79f12d91ef91a469699bfa911f510f to try to work around this problem that
> > showed up on x86_64, but it causes unaligned accesses on sparc64, and is
> > generally a bad idea on 64-bit if RCU pointers are contained within the
> > structure. Moreover, it did not use the same attribute as type attribute, which
> > could cause the iteration on the extern array structure not to match the
> > variable definitions for some structure sizes.
> > 
> > We should also ensure proper alignment of each Ftrace section in
> > include/asm-generic/vmlinux.lds.h.
> > 
> > Moving all STRUCT_ALIGN() for FTRACE_EVENTS() and TRACE_SYSCALLS() into the
> > definitions, so the alignment is only done if these infrastructures are
> > configured in. Use U64_ALIGN instead of STRUCT_ALIGN.
> > 
> > Also align TRACE_PRINTKS on U64_ALIGN to make sure the beginning of the section
> > is aligned on pointer size.
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Thanks! This patch, just like 1/3, should also go into -stable, for both .36 and
.37.

As I'm thinking, the following patch (for Tracepoints) should only go into the
-tip tree, as it does not fix a problem -- it merely reduces the data footprint
of the tracepoints structures. I'm therefore not forwarding patch 3/3 for
inclusion into -stable.

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3)
  2011-01-22 17:11     ` Mathieu Desnoyers
@ 2011-01-22 17:43       ` Steven Rostedt
  2011-01-25 23:34         ` Mathieu Desnoyers
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2011-01-22 17:43 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: David Miller, linux-kernel, fweisbec, mingo, richm, ben,
	sparclinux, stable

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




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3)
  2011-01-22 17:43       ` Steven Rostedt
@ 2011-01-25 23:34         ` Mathieu Desnoyers
  2011-01-26  0:25           ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Mathieu Desnoyers @ 2011-01-25 23:34 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: David Miller, linux-kernel, fweisbec, mingo, richm, ben,
	sparclinux, stable

* 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

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3)
  2011-01-25 23:34         ` Mathieu Desnoyers
@ 2011-01-26  0:25           ` Steven Rostedt
  0 siblings, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2011-01-26  0:25 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: David Miller, linux-kernel, fweisbec, mingo, richm, ben,
	sparclinux, stable

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip:perf/core] Introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections
  2011-01-21 20:36 ` [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3) Mathieu Desnoyers
  2011-01-22  0:05   ` David Miller
@ 2011-01-26  7:12   ` tip-bot for Mathieu Desnoyers
  1 sibling, 0 replies; 16+ messages in thread
From: tip-bot for Mathieu Desnoyers @ 2011-01-26  7:12 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, mathieu.desnoyers, davem, fweisbec,
	rostedt, tglx, mingo

Commit-ID:  3b2c9e246ff913c74d0a11768435845e6106e966
Gitweb:     http://git.kernel.org/tip/3b2c9e246ff913c74d0a11768435845e6106e966
Author:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
AuthorDate: Fri, 21 Jan 2011 15:36:31 -0500
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 21 Jan 2011 22:02:52 -0500

Introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections

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 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 know 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>
LKML-Reference: <20110121203642.725191236@efficios.com>
Acked-by: David Miller <davem@davemloft.net>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |    6 ++++
 include/linux/align-section.h     |   54 +++++++++++++++++++++++++++++++++++++
 include/linux/compiler.h          |    2 +
 3 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6864933..bdc1688 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -70,6 +70,12 @@
 #define STRUCT_ALIGNMENT 32
 #define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
 
+/*
+ * 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)
diff --git a/include/linux/align-section.h b/include/linux/align-section.h
new file mode 100644
index 0000000..6822a4a
--- /dev/null
+++ b/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 */
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 320d6c9..5036024 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -57,6 +57,8 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 # 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip:perf/core] tracing: Fix sparc64 alignment crash with __u64_aligned/U64_ALIGN()
  2011-01-21 20:36 ` [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN() Mathieu Desnoyers
  2011-01-22  0:05   ` David Miller
@ 2011-01-26  7:13   ` tip-bot for Mathieu Desnoyers
  1 sibling, 0 replies; 16+ messages in thread
From: tip-bot for Mathieu Desnoyers @ 2011-01-26  7:13 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, mathieu.desnoyers, davem, fweisbec,
	rostedt, tglx, mingo

Commit-ID:  46396a566597777d7ffa895a6b613e2985c576ff
Gitweb:     http://git.kernel.org/tip/46396a566597777d7ffa895a6b613e2985c576ff
Author:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
AuthorDate: Fri, 21 Jan 2011 15:36:32 -0500
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 21 Jan 2011 22:08:57 -0500

tracing: Fix sparc64 alignment crash with __u64_aligned/U64_ALIGN()

Problem description:

gcc happily align structures defined statically on 32-byte. Ftrace trace events
and Tracepoints both statically define structures into 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 and when they are globally visible and/or in an array.
Therefore iteration on these arrays sees "holes" of padding.

Use the __u64_aligned for type declarations and variable definitions to ensure
that gcc:

a) iterates on the correctly aligned type. (type attribute)
b) generates the definitions within the sections with the appropriate alignment.
   (variable attribute)

The Ftrace code introduced the "aligned(4)" variable attribute in commit
1473e4417c79f12d91ef91a469699bfa911f510f to try to work around this problem that
showed up on x86_64, but it causes unaligned accesses on sparc64, and is
generally a bad idea on 64-bit if RCU pointers are contained within the
structure. Moreover, it did not use the same attribute as type attribute, which
could cause the iteration on the extern array structure not to match the
variable definitions for some structure sizes.

We should also ensure proper alignment of each Ftrace section in
include/asm-generic/vmlinux.lds.h.

Moving all STRUCT_ALIGN() for FTRACE_EVENTS() and TRACE_SYSCALLS() into the
definitions, so the alignment is only done if these infrastructures are
configured in. Use U64_ALIGN instead of STRUCT_ALIGN.

Also align TRACE_PRINTKS on U64_ALIGN to make sure the beginning of the section
is aligned on pointer size.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
LKML-Reference: <20110121203642.884088920@efficios.com>
Acked-by: David Miller <davem@davemloft.net>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |   19 ++++++++++---------
 include/linux/compiler.h          |    6 +++---
 include/linux/ftrace_event.h      |    2 +-
 include/linux/syscalls.h          |   12 ++++++------
 include/trace/ftrace.h            |    8 ++++----
 include/trace/syscall.h           |    2 +-
 kernel/trace/trace.h              |    2 +-
 kernel/trace/trace_export.c       |    2 +-
 8 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index bdc1688..e4af65c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -114,7 +114,8 @@
 #endif
 
 #ifdef CONFIG_TRACE_BRANCH_PROFILING
-#define LIKELY_PROFILE()	VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
+#define LIKELY_PROFILE()	U64_ALIGN();					      \
+				VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
 				*(_ftrace_annotated_branch)			      \
 				VMLINUX_SYMBOL(__stop_annotated_branch_profile) = .;
 #else
@@ -122,7 +123,8 @@
 #endif
 
 #ifdef CONFIG_PROFILE_ALL_BRANCHES
-#define BRANCH_PROFILE()	VMLINUX_SYMBOL(__start_branch_profile) = .;   \
+#define BRANCH_PROFILE()	U64_ALIGN();				      \
+				VMLINUX_SYMBOL(__start_branch_profile) = .;   \
 				*(_ftrace_branch)			      \
 				VMLINUX_SYMBOL(__stop_branch_profile) = .;
 #else
@@ -130,7 +132,8 @@
 #endif
 
 #ifdef CONFIG_EVENT_TRACING
-#define FTRACE_EVENTS()	VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
+#define FTRACE_EVENTS()	U64_ALIGN();					\
+			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
 			*(_ftrace_events)				\
 			VMLINUX_SYMBOL(__stop_ftrace_events) = .;
 #else
@@ -138,7 +141,8 @@
 #endif
 
 #ifdef CONFIG_TRACING
-#define TRACE_PRINTKS() VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .;      \
+#define TRACE_PRINTKS()  U64_ALIGN();					       \
+			 VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .;      \
 			 *(__trace_printk_fmt) /* Trace_printk fmt' pointer */ \
 			 VMLINUX_SYMBOL(__stop___trace_bprintk_fmt) = .;
 #else
@@ -146,7 +150,8 @@
 #endif
 
 #ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
+#define TRACE_SYSCALLS() U64_ALIGN();					\
+			 VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
 			 *(__syscalls_metadata)				\
 			 VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
 #else
@@ -183,11 +188,7 @@
 	LIKELY_PROFILE()		       				\
 	BRANCH_PROFILE()						\
 	TRACE_PRINTKS()							\
-									\
-	STRUCT_ALIGN();							\
 	FTRACE_EVENTS()							\
-									\
-	STRUCT_ALIGN();							\
 	TRACE_SYSCALLS()
 
 /*
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 5036024..ffd6e7e 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -80,7 +80,7 @@ struct ftrace_branch_data {
 		};
 		unsigned long miss_hit[2];
 	};
-};
+} __u64_aligned;
 
 /*
  * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code
@@ -96,7 +96,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #define __branch_check__(x, expect) ({					\
 			int ______r;					\
 			static struct ftrace_branch_data		\
-				__attribute__((__aligned__(4)))		\
+				__u64_aligned				\
 				__attribute__((section("_ftrace_annotated_branch"))) \
 				______f = {				\
 				.func = __func__,			\
@@ -131,7 +131,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 	({								\
 		int ______r;						\
 		static struct ftrace_branch_data			\
-			__attribute__((__aligned__(4)))			\
+			__u64_aligned					\
 			__attribute__((section("_ftrace_branch")))	\
 			______f = {					\
 				.func = __func__,			\
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 47e3997..481a259 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -196,7 +196,7 @@ struct ftrace_event_call {
 	int				perf_refcount;
 	struct hlist_head __percpu	*perf_events;
 #endif
-};
+} __u64_aligned;
 
 #define __TRACE_EVENT_FLAGS(name, value)				\
 	static int __init trace_init_flags_##name(void)			\
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 18cd068..ea7ab85 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -126,9 +126,9 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 
 #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
 	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	  __u64_aligned __syscall_meta_##sname;				\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
+	  __u64_aligned							\
 	  __attribute__((section("_ftrace_events")))			\
 	  event_enter_##sname = {					\
 		.name                   = "sys_enter"#sname,		\
@@ -140,9 +140,9 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
 	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	  __u64_aligned __syscall_meta_##sname;				\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
+	  __u64_aligned							\
 	  __attribute__((section("_ftrace_events")))			\
 	  event_exit_##sname = {					\
 		.name                   = "sys_exit"#sname,		\
@@ -156,7 +156,7 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 	SYSCALL_TRACE_ENTER_EVENT(sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
+	  __u64_aligned						\
 	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta_##sname = {				\
 		.name 		= "sys"#sname,			\
@@ -172,7 +172,7 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 	SYSCALL_TRACE_ENTER_EVENT(_##sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(_##sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
+	  __u64_aligned						\
 	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta__##sname = {				\
 		.name 		= "sys_"#sname,			\
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index e16610c..ee44cf4 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -69,7 +69,7 @@
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, name, proto, args)	\
 	static struct ftrace_event_call	__used		\
-	__attribute__((__aligned__(4))) event_##name
+	__u64_aligned event_##name;
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
@@ -447,7 +447,7 @@ static inline notrace int ftrace_get_offsets_##call(			\
  * };
  *
  * static struct ftrace_event_call __used
- * __attribute__((__aligned__(4)))
+ * __u64_aligned
  * __attribute__((section("_ftrace_events"))) event_<call> = {
  *	.name			= "<call>",
  *	.class			= event_class_<template>,
@@ -580,7 +580,7 @@ static struct ftrace_event_class __used event_class_##call = {		\
 #define DEFINE_EVENT(template, call, proto, args)			\
 									\
 static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
@@ -594,7 +594,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 static const char print_fmt_##call[] = print;				\
 									\
 static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
diff --git a/include/trace/syscall.h b/include/trace/syscall.h
index 31966a4..69578b3 100644
--- a/include/trace/syscall.h
+++ b/include/trace/syscall.h
@@ -29,7 +29,7 @@ struct syscall_metadata {
 
 	struct ftrace_event_call *enter_event;
 	struct ftrace_event_call *exit_event;
-};
+} __u64_aligned;
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 extern unsigned long arch_syscall_addr(int nr);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 9021f8c..6a08891 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -749,7 +749,7 @@ extern const char *__stop___trace_bprintk_fmt[];
 #undef FTRACE_ENTRY
 #define FTRACE_ENTRY(call, struct_name, id, tstruct, print)		\
 	extern struct ftrace_event_call					\
-	__attribute__((__aligned__(4))) event_##call;
+	__u64_aligned event_##call;
 #undef FTRACE_ENTRY_DUP
 #define FTRACE_ENTRY_DUP(call, struct_name, id, tstruct, print)		\
 	FTRACE_ENTRY(call, struct_name, id, PARAMS(tstruct), PARAMS(print))
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index 4b74d71..a6ebf4e 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -162,7 +162,7 @@ struct ftrace_event_class event_class_ftrace_##call = {			\
 };									\
 									\
 struct ftrace_event_call __used						\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.event.type		= etype,				\

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip:perf/core] tracepoints: Use __u64_aligned/U64_ALIGN()
  2011-01-21 20:36 ` [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN() Mathieu Desnoyers
  2011-01-22  0:05   ` David Miller
@ 2011-01-26  7:13   ` tip-bot for Mathieu Desnoyers
  1 sibling, 0 replies; 16+ messages in thread
From: tip-bot for Mathieu Desnoyers @ 2011-01-26  7:13 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, mathieu.desnoyers, davem, fweisbec,
	rostedt, tglx, mingo

Commit-ID:  15e3540ce2159705f18fad6147ffedf04445ad64
Gitweb:     http://git.kernel.org/tip/15e3540ce2159705f18fad6147ffedf04445ad64
Author:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
AuthorDate: Fri, 21 Jan 2011 15:36:33 -0500
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 21 Jan 2011 22:12:13 -0500

tracepoints: Use __u64_aligned/U64_ALIGN()

commit 7e066fb870fcd1025ec3ba7bbde5d541094f4ce1 added the aligned(32) type and
variable attribute to the tracepoint structures to deal with gcc happily
aligning statically defined structures on 32-byte multiples.

Working on issues within Ftrace, we came up with __64_aligned, which deals with
this issue more elegantly by forcing an 8-byte alignment to both the type
declaration and variable definition.

This therefore saves space, bringing down the size of struct tracepoint from 64
bytes to 38 on 64-bit architectures.

Updating:
- The type attribute (for iteration over the struct tracepoint array)
- Added the variable attribute to the extern definition (needed to force gcc to
  consider this alignment for the following definition)
- The definition variable attribute (to force gcc to this specific alignment for
  the static definitions)
- The linker script (8-byte alignment can now replace the previous 32-byte
  alignment for the custom tracepoint section)

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
LKML-Reference: <20110121203643.046218322@efficios.com>
Acked-by: David Miller <davem@davemloft.net>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |    2 +-
 include/linux/tracepoint.h        |   12 ++++--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index e4af65c..95abb5f 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -176,7 +176,7 @@
 	CPU_KEEP(exit.data)						\
 	MEM_KEEP(init.data)						\
 	MEM_KEEP(exit.data)						\
-	. = ALIGN(32);							\
+	U64_ALIGN();							\
 	VMLINUX_SYMBOL(__start___tracepoints) = .;			\
 	*(__tracepoints)						\
 	VMLINUX_SYMBOL(__stop___tracepoints) = .;			\
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index c681461..4bc50ea 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -33,12 +33,7 @@ struct tracepoint {
 	void (*regfunc)(void);
 	void (*unregfunc)(void);
 	struct tracepoint_func __rcu *funcs;
-} __attribute__((aligned(32)));		/*
-					 * Aligned on 32 bytes because it is
-					 * globally visible and gcc happily
-					 * align these on the structure size.
-					 * Keep in sync with vmlinux.lds.h.
-					 */
+} __u64_aligned;
 
 /*
  * Connect a probe to a tracepoint.
@@ -146,7 +141,7 @@ static inline void tracepoint_update_probe_range(struct tracepoint *begin,
  * structure. Force alignment to the same alignment as the section start.
  */
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args)	\
-	extern struct tracepoint __tracepoint_##name;			\
+	extern struct tracepoint __u64_aligned __tracepoint_##name;	\
 	static inline void trace_##name(proto)				\
 	{								\
 		JUMP_LABEL(&__tracepoint_##name.state, do_trace);	\
@@ -178,7 +173,8 @@ do_trace:								\
 	static const char __tpstrtab_##name[]				\
 	__attribute__((section("__tracepoints_strings"))) = #name;	\
 	struct tracepoint __tracepoint_##name				\
-	__attribute__((section("__tracepoints"), aligned(32))) =	\
+	__u64_aligned							\
+	__attribute__((section("__tracepoints"))) =			\
 		{ __tpstrtab_##name, 0, reg, unreg, NULL }
 
 #define DEFINE_TRACE(name)						\

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/3] tracing: Fix sparc64 alignment crash with __u64_aligned/U64_ALIGN()
  2011-01-26  4:04 [PATCH 0/3] [GIT PULL][v2.6.38] tracing: fix unaligned event arrays Steven Rostedt
@ 2011-01-26  4:05 ` Steven Rostedt
  0 siblings, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2011-01-26  4:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, David Miller,
	Mathieu Desnoyers

[-- Attachment #1: 0002-tracing-Fix-sparc64-alignment-crash-with-__u64_align.patch --]
[-- Type: text/plain, Size: 11102 bytes --]

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Problem description:

gcc happily align structures defined statically on 32-byte. Ftrace trace events
and Tracepoints both statically define structures into 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 and when they are globally visible and/or in an array.
Therefore iteration on these arrays sees "holes" of padding.

Use the __u64_aligned for type declarations and variable definitions to ensure
that gcc:

a) iterates on the correctly aligned type. (type attribute)
b) generates the definitions within the sections with the appropriate alignment.
   (variable attribute)

The Ftrace code introduced the "aligned(4)" variable attribute in commit
1473e4417c79f12d91ef91a469699bfa911f510f to try to work around this problem that
showed up on x86_64, but it causes unaligned accesses on sparc64, and is
generally a bad idea on 64-bit if RCU pointers are contained within the
structure. Moreover, it did not use the same attribute as type attribute, which
could cause the iteration on the extern array structure not to match the
variable definitions for some structure sizes.

We should also ensure proper alignment of each Ftrace section in
include/asm-generic/vmlinux.lds.h.

Moving all STRUCT_ALIGN() for FTRACE_EVENTS() and TRACE_SYSCALLS() into the
definitions, so the alignment is only done if these infrastructures are
configured in. Use U64_ALIGN instead of STRUCT_ALIGN.

Also align TRACE_PRINTKS on U64_ALIGN to make sure the beginning of the section
is aligned on pointer size.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
LKML-Reference: <20110121203642.884088920@efficios.com>
Acked-by: David Miller <davem@davemloft.net>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |   19 ++++++++++---------
 include/linux/compiler.h          |    6 +++---
 include/linux/ftrace_event.h      |    2 +-
 include/linux/syscalls.h          |   12 ++++++------
 include/trace/ftrace.h            |    8 ++++----
 include/trace/syscall.h           |    2 +-
 kernel/trace/trace.h              |    2 +-
 kernel/trace/trace_export.c       |    2 +-
 8 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index bdc1688..e4af65c 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -114,7 +114,8 @@
 #endif
 
 #ifdef CONFIG_TRACE_BRANCH_PROFILING
-#define LIKELY_PROFILE()	VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
+#define LIKELY_PROFILE()	U64_ALIGN();					      \
+				VMLINUX_SYMBOL(__start_annotated_branch_profile) = .; \
 				*(_ftrace_annotated_branch)			      \
 				VMLINUX_SYMBOL(__stop_annotated_branch_profile) = .;
 #else
@@ -122,7 +123,8 @@
 #endif
 
 #ifdef CONFIG_PROFILE_ALL_BRANCHES
-#define BRANCH_PROFILE()	VMLINUX_SYMBOL(__start_branch_profile) = .;   \
+#define BRANCH_PROFILE()	U64_ALIGN();				      \
+				VMLINUX_SYMBOL(__start_branch_profile) = .;   \
 				*(_ftrace_branch)			      \
 				VMLINUX_SYMBOL(__stop_branch_profile) = .;
 #else
@@ -130,7 +132,8 @@
 #endif
 
 #ifdef CONFIG_EVENT_TRACING
-#define FTRACE_EVENTS()	VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
+#define FTRACE_EVENTS()	U64_ALIGN();					\
+			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
 			*(_ftrace_events)				\
 			VMLINUX_SYMBOL(__stop_ftrace_events) = .;
 #else
@@ -138,7 +141,8 @@
 #endif
 
 #ifdef CONFIG_TRACING
-#define TRACE_PRINTKS() VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .;      \
+#define TRACE_PRINTKS()  U64_ALIGN();					       \
+			 VMLINUX_SYMBOL(__start___trace_bprintk_fmt) = .;      \
 			 *(__trace_printk_fmt) /* Trace_printk fmt' pointer */ \
 			 VMLINUX_SYMBOL(__stop___trace_bprintk_fmt) = .;
 #else
@@ -146,7 +150,8 @@
 #endif
 
 #ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
+#define TRACE_SYSCALLS() U64_ALIGN();					\
+			 VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
 			 *(__syscalls_metadata)				\
 			 VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
 #else
@@ -183,11 +188,7 @@
 	LIKELY_PROFILE()		       				\
 	BRANCH_PROFILE()						\
 	TRACE_PRINTKS()							\
-									\
-	STRUCT_ALIGN();							\
 	FTRACE_EVENTS()							\
-									\
-	STRUCT_ALIGN();							\
 	TRACE_SYSCALLS()
 
 /*
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 5036024..ffd6e7e 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -80,7 +80,7 @@ struct ftrace_branch_data {
 		};
 		unsigned long miss_hit[2];
 	};
-};
+} __u64_aligned;
 
 /*
  * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code
@@ -96,7 +96,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #define __branch_check__(x, expect) ({					\
 			int ______r;					\
 			static struct ftrace_branch_data		\
-				__attribute__((__aligned__(4)))		\
+				__u64_aligned				\
 				__attribute__((section("_ftrace_annotated_branch"))) \
 				______f = {				\
 				.func = __func__,			\
@@ -131,7 +131,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 	({								\
 		int ______r;						\
 		static struct ftrace_branch_data			\
-			__attribute__((__aligned__(4)))			\
+			__u64_aligned					\
 			__attribute__((section("_ftrace_branch")))	\
 			______f = {					\
 				.func = __func__,			\
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 47e3997..481a259 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -196,7 +196,7 @@ struct ftrace_event_call {
 	int				perf_refcount;
 	struct hlist_head __percpu	*perf_events;
 #endif
-};
+} __u64_aligned;
 
 #define __TRACE_EVENT_FLAGS(name, value)				\
 	static int __init trace_init_flags_##name(void)			\
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 18cd068..ea7ab85 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -126,9 +126,9 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 
 #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
 	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	  __u64_aligned __syscall_meta_##sname;				\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
+	  __u64_aligned							\
 	  __attribute__((section("_ftrace_events")))			\
 	  event_enter_##sname = {					\
 		.name                   = "sys_enter"#sname,		\
@@ -140,9 +140,9 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
 	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	  __u64_aligned __syscall_meta_##sname;				\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
+	  __u64_aligned							\
 	  __attribute__((section("_ftrace_events")))			\
 	  event_exit_##sname = {					\
 		.name                   = "sys_exit"#sname,		\
@@ -156,7 +156,7 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 	SYSCALL_TRACE_ENTER_EVENT(sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
+	  __u64_aligned						\
 	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta_##sname = {				\
 		.name 		= "sys"#sname,			\
@@ -172,7 +172,7 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 	SYSCALL_TRACE_ENTER_EVENT(_##sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(_##sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
+	  __u64_aligned						\
 	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta__##sname = {				\
 		.name 		= "sys_"#sname,			\
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index e16610c..ee44cf4 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -69,7 +69,7 @@
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, name, proto, args)	\
 	static struct ftrace_event_call	__used		\
-	__attribute__((__aligned__(4))) event_##name
+	__u64_aligned event_##name;
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
@@ -447,7 +447,7 @@ static inline notrace int ftrace_get_offsets_##call(			\
  * };
  *
  * static struct ftrace_event_call __used
- * __attribute__((__aligned__(4)))
+ * __u64_aligned
  * __attribute__((section("_ftrace_events"))) event_<call> = {
  *	.name			= "<call>",
  *	.class			= event_class_<template>,
@@ -580,7 +580,7 @@ static struct ftrace_event_class __used event_class_##call = {		\
 #define DEFINE_EVENT(template, call, proto, args)			\
 									\
 static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
@@ -594,7 +594,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 static const char print_fmt_##call[] = print;				\
 									\
 static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
diff --git a/include/trace/syscall.h b/include/trace/syscall.h
index 31966a4..69578b3 100644
--- a/include/trace/syscall.h
+++ b/include/trace/syscall.h
@@ -29,7 +29,7 @@ struct syscall_metadata {
 
 	struct ftrace_event_call *enter_event;
 	struct ftrace_event_call *exit_event;
-};
+} __u64_aligned;
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 extern unsigned long arch_syscall_addr(int nr);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 9021f8c..6a08891 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -749,7 +749,7 @@ extern const char *__stop___trace_bprintk_fmt[];
 #undef FTRACE_ENTRY
 #define FTRACE_ENTRY(call, struct_name, id, tstruct, print)		\
 	extern struct ftrace_event_call					\
-	__attribute__((__aligned__(4))) event_##call;
+	__u64_aligned event_##call;
 #undef FTRACE_ENTRY_DUP
 #define FTRACE_ENTRY_DUP(call, struct_name, id, tstruct, print)		\
 	FTRACE_ENTRY(call, struct_name, id, PARAMS(tstruct), PARAMS(print))
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index 4b74d71..a6ebf4e 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -162,7 +162,7 @@ struct ftrace_event_class event_class_ftrace_##call = {			\
 };									\
 									\
 struct ftrace_event_call __used						\
-__attribute__((__aligned__(4)))						\
+__u64_aligned								\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.event.type		= etype,				\
-- 
1.7.2.3



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-01-26  7:14 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-21 20:36 [patch 0/3] Fix alignment of custom sections made from structures (v3) Mathieu Desnoyers
2011-01-21 20:36 ` [patch 1/3] introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections (v3) Mathieu Desnoyers
2011-01-22  0:05   ` David Miller
2011-01-22 17:11     ` Mathieu Desnoyers
2011-01-22 17:43       ` Steven Rostedt
2011-01-25 23:34         ` Mathieu Desnoyers
2011-01-26  0:25           ` Steven Rostedt
2011-01-26  7:12   ` [tip:perf/core] Introduce __u64_aligned and U64_ALIGN() for structure alignment in custom sections tip-bot for Mathieu Desnoyers
2011-01-21 20:36 ` [patch 2/3] tracing: fix sparc64 alignment crash with __u64_aligned/U64_ALIGN() Mathieu Desnoyers
2011-01-22  0:05   ` David Miller
2011-01-22 17:15     ` Mathieu Desnoyers
2011-01-26  7:13   ` [tip:perf/core] tracing: Fix " tip-bot for Mathieu Desnoyers
2011-01-21 20:36 ` [patch 3/3] tracepoints: use __u64_aligned/U64_ALIGN() Mathieu Desnoyers
2011-01-22  0:05   ` David Miller
2011-01-26  7:13   ` [tip:perf/core] tracepoints: Use __u64_aligned/U64_ALIGN() tip-bot for Mathieu Desnoyers
2011-01-26  4:04 [PATCH 0/3] [GIT PULL][v2.6.38] tracing: fix unaligned event arrays Steven Rostedt
2011-01-26  4:05 ` [PATCH 2/3] tracing: Fix sparc64 alignment crash with __u64_aligned/U64_ALIGN() Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).