[PATCH 0/2] [GIT PULL][v2.6.38]: tracing: alignment fixes (take 2)

[PATCH 0/2] [GIT PULL][v2.6.38]: tracing: alignment fixes (take 2)

[Steven Rostedt]

Ingo,

OK, this time I ran this under x86_64 and x86_32 with gcc-4.5.1
and gcc-4.4.5. It even found a bug, that I had to fix and retest.

Mathieu,

Can you review your patch, as it had some conflicts (Ingo removed
the other broken patch set which you based this on). Just make sure
that I didn't screw anything up.

David,

Can you test this on sparc?

Please pull the latest tip/perf/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/urgent


Mathieu Desnoyers (1):
      Tracepoints: Fix section alignment using pointer array

Steven Rostedt (1):
      tracing: Replace trace_event struct array with pointer array

----
 include/asm-generic/vmlinux.lds.h |   15 ++++++++-------
 include/linux/module.h            |    4 ++--
 include/linux/syscalls.h          |   10 ++++++----
 include/linux/tracepoint.h        |   35 ++++++++++++++++++++---------------
 include/trace/ftrace.h            |   24 +++++++++++++-----------
 kernel/module.c                   |   16 ++++++++--------
 kernel/trace/trace_events.c       |   12 ++++++------
 kernel/trace/trace_export.c       |    6 +++---
 kernel/tracepoint.c               |   31 ++++++++++++++++---------------
 9 files changed, 82 insertions(+), 71 deletions(-)

[PATCH 1/2] tracing: Replace trace_event struct array with pointer array

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Replace-trace_event-struct-array-with-pointe.patch --]
[-- Type: text/plain, Size: 9931 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Currently the trace_event structures are placed in the _ftrace_events
section, and at link time, the linker makes one large array of all
the trace_event structures. On boot up, this array is read (much like
the initcall sections) and the events are processed.

The problem is that there is no guarantee that gcc will place complex
structures nicely together in an array format. Two structures in the
same file may be placed awkwardly, because gcc has no clue that they
are suppose to be in an array.

A hack was used previous to force the alignment to 4, to pack the
structures together. But this caused alignment issues with other
architectures (sparc).

Instead of packing the structures into an array, the structures' addresses
are now put into the _ftrace_event section. As pointers are always the
natural alignment, gcc should always pack them tightly together
(otherwise initcall, extable, etc would also fail).

By having the pointers to the structures in the section, we can still
iterate the trace_events without causing unnecessary alignment problems
with other architectures, or depending on the current behaviour of
gcc that will likely change in the future just to tick us kernel developers
off a little more.

The _ftrace_event section is also moved into the .init.data section
as it is now only needed at boot up.

Suggested-by: David Miller <davem@davemloft.net>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |    7 +++----
 include/linux/module.h            |    2 +-
 include/linux/syscalls.h          |   10 ++++++----
 include/trace/ftrace.h            |   24 +++++++++++++-----------
 kernel/trace/trace_events.c       |   12 ++++++------
 kernel/trace/trace_export.c       |    6 +++---
 6 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 6ebb810..f53708b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -124,7 +124,8 @@
 #endif
 
 #ifdef CONFIG_EVENT_TRACING
-#define FTRACE_EVENTS()	VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
+#define FTRACE_EVENTS()	. = ALIGN(8);					\
+			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
 			*(_ftrace_events)				\
 			VMLINUX_SYMBOL(__stop_ftrace_events) = .;
 #else
@@ -179,9 +180,6 @@
 	TRACE_PRINTKS()							\
 									\
 	STRUCT_ALIGN();							\
-	FTRACE_EVENTS()							\
-									\
-	STRUCT_ALIGN();							\
 	TRACE_SYSCALLS()
 
 /*
@@ -482,6 +480,7 @@
 	KERNEL_CTORS()							\
 	*(.init.rodata)							\
 	MCOUNT_REC()							\
+	FTRACE_EVENTS()							\
 	DEV_DISCARD(init.rodata)					\
 	CPU_DISCARD(init.rodata)					\
 	MEM_DISCARD(init.rodata)					\
diff --git a/include/linux/module.h b/include/linux/module.h
index e7c6385..7695a30 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -389,7 +389,7 @@ struct module
 	unsigned int num_trace_bprintk_fmt;
 #endif
 #ifdef CONFIG_EVENT_TRACING
-	struct ftrace_event_call *trace_events;
+	struct ftrace_event_call **trace_events;
 	unsigned int num_trace_events;
 #endif
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 18cd068..45508fe 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -128,28 +128,30 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 	static struct syscall_metadata					\
 	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
-	  __attribute__((section("_ftrace_events")))			\
 	  event_enter_##sname = {					\
 		.name                   = "sys_enter"#sname,		\
 		.class			= &event_class_syscall_enter,	\
 		.event.funcs            = &enter_syscall_print_funcs,	\
 		.data			= (void *)&__syscall_meta_##sname,\
 	};								\
+	static struct ftrace_event_call __used				\
+	  __attribute__((section("_ftrace_events")))			\
+	 *__event_enter_##sname = &event_enter_##sname;			\
 	__TRACE_EVENT_FLAGS(enter_##sname, TRACE_EVENT_FL_CAP_ANY)
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
 	static struct syscall_metadata					\
 	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
 	static struct ftrace_event_call __used				\
-	  __attribute__((__aligned__(4)))				\
-	  __attribute__((section("_ftrace_events")))			\
 	  event_exit_##sname = {					\
 		.name                   = "sys_exit"#sname,		\
 		.class			= &event_class_syscall_exit,	\
 		.event.funcs		= &exit_syscall_print_funcs,	\
 		.data			= (void *)&__syscall_meta_##sname,\
 	};								\
+	static struct ftrace_event_call __used				\
+	  __attribute__((section("_ftrace_events")))			\
+	*__event_exit_##sname = &event_exit_##sname;			\
 	__TRACE_EVENT_FLAGS(exit_##sname, TRACE_EVENT_FL_CAP_ANY)
 
 #define SYSCALL_METADATA(sname, nb)				\
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index e16610c..3e68366 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -446,14 +446,16 @@ static inline notrace int ftrace_get_offsets_##call(			\
  *	.reg			= ftrace_event_reg,
  * };
  *
- * static struct ftrace_event_call __used
- * __attribute__((__aligned__(4)))
- * __attribute__((section("_ftrace_events"))) event_<call> = {
+ * static struct ftrace_event_call event_<call> = {
  *	.name			= "<call>",
  *	.class			= event_class_<template>,
  *	.event			= &ftrace_event_type_<call>,
  *	.print_fmt		= print_fmt_<call>,
  * };
+ * // its only safe to use pointers when doing linker tricks to
+ * // create an array.
+ * static struct ftrace_event_call __used
+ * __attribute__((section("_ftrace_events"))) *__event_<call> = &event_<call>;
  *
  */
 
@@ -579,28 +581,28 @@ static struct ftrace_event_class __used event_class_##call = {		\
 #undef DEFINE_EVENT
 #define DEFINE_EVENT(template, call, proto, args)			\
 									\
-static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
-__attribute__((section("_ftrace_events"))) event_##call = {		\
+static struct ftrace_event_call __used event_##call = {			\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
 	.event.funcs		= &ftrace_event_type_funcs_##template,	\
 	.print_fmt		= print_fmt_##template,			\
-};
+};									\
+static struct ftrace_event_call __used					\
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, call, proto, args, print)		\
 									\
 static const char print_fmt_##call[] = print;				\
 									\
-static struct ftrace_event_call __used					\
-__attribute__((__aligned__(4)))						\
-__attribute__((section("_ftrace_events"))) event_##call = {		\
+static struct ftrace_event_call __used event_##call = {			\
 	.name			= #call,				\
 	.class			= &event_class_##template,		\
 	.event.funcs		= &ftrace_event_type_funcs_##call,	\
 	.print_fmt		= print_fmt_##call,			\
-}
+};									\
+static struct ftrace_event_call __used					\
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
 
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 35fde09..5f499e0 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1284,7 +1284,7 @@ trace_create_file_ops(struct module *mod)
 static void trace_module_add_events(struct module *mod)
 {
 	struct ftrace_module_file_ops *file_ops = NULL;
-	struct ftrace_event_call *call, *start, *end;
+	struct ftrace_event_call **call, **start, **end;
 
 	start = mod->trace_events;
 	end = mod->trace_events + mod->num_trace_events;
@@ -1297,7 +1297,7 @@ static void trace_module_add_events(struct module *mod)
 		return;
 
 	for_each_event(call, start, end) {
-		__trace_add_event_call(call, mod,
+		__trace_add_event_call(*call, mod,
 				       &file_ops->id, &file_ops->enable,
 				       &file_ops->filter, &file_ops->format);
 	}
@@ -1367,8 +1367,8 @@ static struct notifier_block trace_module_nb = {
 	.priority = 0,
 };
 
-extern struct ftrace_event_call __start_ftrace_events[];
-extern struct ftrace_event_call __stop_ftrace_events[];
+extern struct ftrace_event_call *__start_ftrace_events[];
+extern struct ftrace_event_call *__stop_ftrace_events[];
 
 static char bootup_event_buf[COMMAND_LINE_SIZE] __initdata;
 
@@ -1384,7 +1384,7 @@ __setup("trace_event=", setup_trace_event);
 
 static __init int event_trace_init(void)
 {
-	struct ftrace_event_call *call;
+	struct ftrace_event_call **call;
 	struct dentry *d_tracer;
 	struct dentry *entry;
 	struct dentry *d_events;
@@ -1430,7 +1430,7 @@ static __init int event_trace_init(void)
 		pr_warning("tracing: Failed to allocate common fields");
 
 	for_each_event(call, __start_ftrace_events, __stop_ftrace_events) {
-		__trace_add_event_call(call, NULL, &ftrace_event_id_fops,
+		__trace_add_event_call(*call, NULL, &ftrace_event_id_fops,
 				       &ftrace_enable_fops,
 				       &ftrace_event_filter_fops,
 				       &ftrace_event_format_fops);
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index 4b74d71..bbeec31 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -161,13 +161,13 @@ struct ftrace_event_class event_class_ftrace_##call = {			\
 	.fields			= LIST_HEAD_INIT(event_class_ftrace_##call.fields),\
 };									\
 									\
-struct ftrace_event_call __used						\
-__attribute__((__aligned__(4)))						\
-__attribute__((section("_ftrace_events"))) event_##call = {		\
+struct ftrace_event_call __used event_##call = {			\
 	.name			= #call,				\
 	.event.type		= etype,				\
 	.class			= &event_class_ftrace_##call,		\
 	.print_fmt		= print,				\
 };									\
+struct ftrace_event_call __used						\
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call;
 
 #include "trace_entries.h"
-- 
1.7.2.3

[PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Steven Rostedt]

[-- Attachment #1: 0002-Tracepoints-Fix-section-alignment-using-pointer-arra.patch --]
[-- Type: text/plain, Size: 11493 bytes --]

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

Make the tracepoints more robust, making them solid enough to handle compiler
changes by not relying on anything based on compiler-specific behavior with
respect to structure alignment. Implement an approach proposed by David Miller:
use an array of const pointers to refer to the individual structures, and export
this pointer array through the linker script rather than the structures per se.
It will consume 32 extra bytes per tracepoint (24 for structure padding and 8
for the pointers), but are less likely to break due to compiler changes.

History:

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.

commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
for tracepoint structures by applying both the variable and type attribute to
tracepoint structures definitions and declarations. It worked fine with gcc
4.5.1, but broke with gcc 4.4.4 and 4.4.5.

The reason is that the "aligned" attribute only specify the _minimum_ alignment
for a structure, leaving both the compiler and the linker free to align on
larger multiples. Because tracepoint.c expects the structures to be placed as an
array within each section, up-alignment cause NULL-pointer exceptions due to the
extra unexpected padding.

(this patch applies on top of -tip)

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
LKML-Reference: <20110126222622.GA10794@Krystal>
CC: David Miller <davem@davemloft.net>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |    8 +++++---
 include/linux/module.h            |    2 +-
 include/linux/tracepoint.h        |   35 ++++++++++++++++++++---------------
 kernel/module.c                   |   16 ++++++++--------
 kernel/tracepoint.c               |   31 ++++++++++++++++---------------
 5 files changed, 50 insertions(+), 42 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f53708b..57b1b68 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -166,10 +166,8 @@
 	CPU_KEEP(exit.data)						\
 	MEM_KEEP(init.data)						\
 	MEM_KEEP(exit.data)						\
-	. = ALIGN(32);							\
-	VMLINUX_SYMBOL(__start___tracepoints) = .;			\
+	STRUCT_ALIGN();							\
 	*(__tracepoints)						\
-	VMLINUX_SYMBOL(__stop___tracepoints) = .;			\
 	/* implement dynamic printk debug */				\
 	. = ALIGN(8);							\
 	VMLINUX_SYMBOL(__start___verbose) = .;                          \
@@ -218,6 +216,10 @@
 		VMLINUX_SYMBOL(__start_rodata) = .;			\
 		*(.rodata) *(.rodata.*)					\
 		*(__vermagic)		/* Kernel version magic */	\
+		. = ALIGN(8);						\
+		VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;		\
+		*(__tracepoints_ptrs)	/* Tracepoints: pointer array */\
+		VMLINUX_SYMBOL(__stop___tracepoints_ptrs) = .;		\
 		*(__markers_strings)	/* Markers: strings */		\
 		*(__tracepoints_strings)/* Tracepoints: strings */	\
 	}								\
diff --git a/include/linux/module.h b/include/linux/module.h
index 7695a30..9bdf27c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -377,7 +377,7 @@ struct module
 	   keeping pointers to this stuff */
 	char *args;
 #ifdef CONFIG_TRACEPOINTS
-	struct tracepoint *tracepoints;
+	struct tracepoint * const *tracepoints_ptrs;
 	unsigned int num_tracepoints;
 #endif
 #ifdef HAVE_JUMP_LABEL
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index c681461..97c84a5 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.
-					 */
+};
 
 /*
  * Connect a probe to a tracepoint.
@@ -61,15 +56,15 @@ extern void tracepoint_probe_update_all(void);
 
 struct tracepoint_iter {
 	struct module *module;
-	struct tracepoint *tracepoint;
+	struct tracepoint * const *tracepoint;
 };
 
 extern void tracepoint_iter_start(struct tracepoint_iter *iter);
 extern void tracepoint_iter_next(struct tracepoint_iter *iter);
 extern void tracepoint_iter_stop(struct tracepoint_iter *iter);
 extern void tracepoint_iter_reset(struct tracepoint_iter *iter);
-extern int tracepoint_get_iter_range(struct tracepoint **tracepoint,
-	struct tracepoint *begin, struct tracepoint *end);
+extern int tracepoint_get_iter_range(struct tracepoint * const **tracepoint,
+	struct tracepoint * const *begin, struct tracepoint * const *end);
 
 /*
  * tracepoint_synchronize_unregister must be called between the last tracepoint
@@ -84,11 +79,13 @@ static inline void tracepoint_synchronize_unregister(void)
 #define PARAMS(args...) args
 
 #ifdef CONFIG_TRACEPOINTS
-extern void tracepoint_update_probe_range(struct tracepoint *begin,
-	struct tracepoint *end);
+extern
+void tracepoint_update_probe_range(struct tracepoint * const *begin,
+	struct tracepoint * const *end);
 #else
-static inline void tracepoint_update_probe_range(struct tracepoint *begin,
-	struct tracepoint *end)
+static inline
+void tracepoint_update_probe_range(struct tracepoint * const *begin,
+	struct tracepoint * const *end)
 { }
 #endif /* CONFIG_TRACEPOINTS */
 
@@ -174,12 +171,20 @@ do_trace:								\
 	{								\
 	}
 
+/*
+ * We have no guarantee that gcc and the linker won't up-align the tracepoint
+ * structures, so we create an array of pointers that will be used for iteration
+ * on the tracepoints.
+ */
 #define DEFINE_TRACE_FN(name, reg, unreg)				\
 	static const char __tpstrtab_##name[]				\
 	__attribute__((section("__tracepoints_strings"))) = #name;	\
 	struct tracepoint __tracepoint_##name				\
-	__attribute__((section("__tracepoints"), aligned(32))) =	\
-		{ __tpstrtab_##name, 0, reg, unreg, NULL }
+	__attribute__((section("__tracepoints"))) =			\
+		{ __tpstrtab_##name, 0, reg, unreg, NULL };		\
+	static struct tracepoint * const __tracepoint_ptr_##name __used	\
+	__attribute__((section("__tracepoints_ptrs"))) =		\
+		&__tracepoint_##name;
 
 #define DEFINE_TRACE(name)						\
 	DEFINE_TRACE_FN(name, NULL, NULL);
diff --git a/kernel/module.c b/kernel/module.c
index 34e00b7..efa290e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2460,9 +2460,9 @@ static void find_module_sections(struct module *mod, struct load_info *info)
 #endif
 
 #ifdef CONFIG_TRACEPOINTS
-	mod->tracepoints = section_objs(info, "__tracepoints",
-					sizeof(*mod->tracepoints),
-					&mod->num_tracepoints);
+	mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
+					     sizeof(*mod->tracepoints_ptrs),
+					     &mod->num_tracepoints);
 #endif
 #ifdef HAVE_JUMP_LABEL
 	mod->jump_entries = section_objs(info, "__jump_table",
@@ -3393,7 +3393,7 @@ void module_layout(struct module *mod,
 		   struct modversion_info *ver,
 		   struct kernel_param *kp,
 		   struct kernel_symbol *ks,
-		   struct tracepoint *tp)
+		   struct tracepoint * const *tp)
 {
 }
 EXPORT_SYMBOL(module_layout);
@@ -3407,8 +3407,8 @@ void module_update_tracepoints(void)
 	mutex_lock(&module_mutex);
 	list_for_each_entry(mod, &modules, list)
 		if (!mod->taints)
-			tracepoint_update_probe_range(mod->tracepoints,
-				mod->tracepoints + mod->num_tracepoints);
+			tracepoint_update_probe_range(mod->tracepoints_ptrs,
+				mod->tracepoints_ptrs + mod->num_tracepoints);
 	mutex_unlock(&module_mutex);
 }
 
@@ -3432,8 +3432,8 @@ int module_get_iter_tracepoints(struct tracepoint_iter *iter)
 			else if (iter_mod > iter->module)
 				iter->tracepoint = NULL;
 			found = tracepoint_get_iter_range(&iter->tracepoint,
-				iter_mod->tracepoints,
-				iter_mod->tracepoints
+				iter_mod->tracepoints_ptrs,
+				iter_mod->tracepoints_ptrs
 					+ iter_mod->num_tracepoints);
 			if (found) {
 				iter->module = iter_mod;
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index e95ee7f..68187af 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -27,8 +27,8 @@
 #include <linux/sched.h>
 #include <linux/jump_label.h>
 
-extern struct tracepoint __start___tracepoints[];
-extern struct tracepoint __stop___tracepoints[];
+extern struct tracepoint * const __start___tracepoints_ptrs[];
+extern struct tracepoint * const __stop___tracepoints_ptrs[];
 
 /* Set to 1 to enable tracepoint debug output */
 static const int tracepoint_debug;
@@ -298,10 +298,10 @@ static void disable_tracepoint(struct tracepoint *elem)
  *
  * Updates the probe callback corresponding to a range of tracepoints.
  */
-void
-tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
+void tracepoint_update_probe_range(struct tracepoint * const *begin,
+				   struct tracepoint * const *end)
 {
-	struct tracepoint *iter;
+	struct tracepoint * const *iter;
 	struct tracepoint_entry *mark_entry;
 
 	if (!begin)
@@ -309,12 +309,12 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
 
 	mutex_lock(&tracepoints_mutex);
 	for (iter = begin; iter < end; iter++) {
-		mark_entry = get_tracepoint(iter->name);
+		mark_entry = get_tracepoint((*iter)->name);
 		if (mark_entry) {
-			set_tracepoint(&mark_entry, iter,
+			set_tracepoint(&mark_entry, *iter,
 					!!mark_entry->refcount);
 		} else {
-			disable_tracepoint(iter);
+			disable_tracepoint(*iter);
 		}
 	}
 	mutex_unlock(&tracepoints_mutex);
@@ -326,8 +326,8 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
 static void tracepoint_update_probes(void)
 {
 	/* Core kernel tracepoints */
-	tracepoint_update_probe_range(__start___tracepoints,
-		__stop___tracepoints);
+	tracepoint_update_probe_range(__start___tracepoints_ptrs,
+		__stop___tracepoints_ptrs);
 	/* tracepoints in modules. */
 	module_update_tracepoints();
 }
@@ -514,8 +514,8 @@ EXPORT_SYMBOL_GPL(tracepoint_probe_update_all);
  * Will return the first tracepoint in the range if the input tracepoint is
  * NULL.
  */
-int tracepoint_get_iter_range(struct tracepoint **tracepoint,
-	struct tracepoint *begin, struct tracepoint *end)
+int tracepoint_get_iter_range(struct tracepoint * const **tracepoint,
+	struct tracepoint * const *begin, struct tracepoint * const *end)
 {
 	if (!*tracepoint && begin != end) {
 		*tracepoint = begin;
@@ -534,7 +534,8 @@ static void tracepoint_get_iter(struct tracepoint_iter *iter)
 	/* Core kernel tracepoints */
 	if (!iter->module) {
 		found = tracepoint_get_iter_range(&iter->tracepoint,
-				__start___tracepoints, __stop___tracepoints);
+				__start___tracepoints_ptrs,
+				__stop___tracepoints_ptrs);
 		if (found)
 			goto end;
 	}
@@ -585,8 +586,8 @@ int tracepoint_module_notify(struct notifier_block *self,
 	switch (val) {
 	case MODULE_STATE_COMING:
 	case MODULE_STATE_GOING:
-		tracepoint_update_probe_range(mod->tracepoints,
-			mod->tracepoints + mod->num_tracepoints);
+		tracepoint_update_probe_range(mod->tracepoints_ptrs,
+			mod->tracepoints_ptrs + mod->num_tracepoints);
 		break;
 	}
 	return 0;
-- 
1.7.2.3

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> Make the tracepoints more robust, making them solid enough to handle compiler
> changes by not relying on anything based on compiler-specific behavior with
> respect to structure alignment. Implement an approach proposed by David Miller:
> use an array of const pointers to refer to the individual structures, and export
> this pointer array through the linker script rather than the structures per se.
> It will consume 32 extra bytes per tracepoint (24 for structure padding and 8
> for the pointers), but are less likely to break due to compiler changes.
> 
> History:
> 
> 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.
> 
> commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
> for tracepoint structures by applying both the variable and type attribute to
> tracepoint structures definitions and declarations. It worked fine with gcc
> 4.5.1, but broke with gcc 4.4.4 and 4.4.5.

Small nit: this reference to commit 15e3540ce2159705f18fad6147ffedf04445ad64
should be changed to a non-commit-id-related explanation, because the commit has
been rolled back from -tip.

The rest looks good.

Thanks,

Mathieu

> 
> The reason is that the "aligned" attribute only specify the _minimum_ alignment
> for a structure, leaving both the compiler and the linker free to align on
> larger multiples. Because tracepoint.c expects the structures to be placed as an
> array within each section, up-alignment cause NULL-pointer exceptions due to the
> extra unexpected padding.
> 
> (this patch applies on top of -tip)
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> LKML-Reference: <20110126222622.GA10794@Krystal>
> CC: David Miller <davem@davemloft.net>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/asm-generic/vmlinux.lds.h |    8 +++++---
>  include/linux/module.h            |    2 +-
>  include/linux/tracepoint.h        |   35 ++++++++++++++++++++---------------
>  kernel/module.c                   |   16 ++++++++--------
>  kernel/tracepoint.c               |   31 ++++++++++++++++---------------
>  5 files changed, 50 insertions(+), 42 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index f53708b..57b1b68 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -166,10 +166,8 @@
>  	CPU_KEEP(exit.data)						\
>  	MEM_KEEP(init.data)						\
>  	MEM_KEEP(exit.data)						\
> -	. = ALIGN(32);							\
> -	VMLINUX_SYMBOL(__start___tracepoints) = .;			\
> +	STRUCT_ALIGN();							\
>  	*(__tracepoints)						\
> -	VMLINUX_SYMBOL(__stop___tracepoints) = .;			\
>  	/* implement dynamic printk debug */				\
>  	. = ALIGN(8);							\
>  	VMLINUX_SYMBOL(__start___verbose) = .;                          \
> @@ -218,6 +216,10 @@
>  		VMLINUX_SYMBOL(__start_rodata) = .;			\
>  		*(.rodata) *(.rodata.*)					\
>  		*(__vermagic)		/* Kernel version magic */	\
> +		. = ALIGN(8);						\
> +		VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;		\
> +		*(__tracepoints_ptrs)	/* Tracepoints: pointer array */\
> +		VMLINUX_SYMBOL(__stop___tracepoints_ptrs) = .;		\
>  		*(__markers_strings)	/* Markers: strings */		\
>  		*(__tracepoints_strings)/* Tracepoints: strings */	\
>  	}								\
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7695a30..9bdf27c 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -377,7 +377,7 @@ struct module
>  	   keeping pointers to this stuff */
>  	char *args;
>  #ifdef CONFIG_TRACEPOINTS
> -	struct tracepoint *tracepoints;
> +	struct tracepoint * const *tracepoints_ptrs;
>  	unsigned int num_tracepoints;
>  #endif
>  #ifdef HAVE_JUMP_LABEL
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index c681461..97c84a5 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.
> -					 */
> +};
>  
>  /*
>   * Connect a probe to a tracepoint.
> @@ -61,15 +56,15 @@ extern void tracepoint_probe_update_all(void);
>  
>  struct tracepoint_iter {
>  	struct module *module;
> -	struct tracepoint *tracepoint;
> +	struct tracepoint * const *tracepoint;
>  };
>  
>  extern void tracepoint_iter_start(struct tracepoint_iter *iter);
>  extern void tracepoint_iter_next(struct tracepoint_iter *iter);
>  extern void tracepoint_iter_stop(struct tracepoint_iter *iter);
>  extern void tracepoint_iter_reset(struct tracepoint_iter *iter);
> -extern int tracepoint_get_iter_range(struct tracepoint **tracepoint,
> -	struct tracepoint *begin, struct tracepoint *end);
> +extern int tracepoint_get_iter_range(struct tracepoint * const **tracepoint,
> +	struct tracepoint * const *begin, struct tracepoint * const *end);
>  
>  /*
>   * tracepoint_synchronize_unregister must be called between the last tracepoint
> @@ -84,11 +79,13 @@ static inline void tracepoint_synchronize_unregister(void)
>  #define PARAMS(args...) args
>  
>  #ifdef CONFIG_TRACEPOINTS
> -extern void tracepoint_update_probe_range(struct tracepoint *begin,
> -	struct tracepoint *end);
> +extern
> +void tracepoint_update_probe_range(struct tracepoint * const *begin,
> +	struct tracepoint * const *end);
>  #else
> -static inline void tracepoint_update_probe_range(struct tracepoint *begin,
> -	struct tracepoint *end)
> +static inline
> +void tracepoint_update_probe_range(struct tracepoint * const *begin,
> +	struct tracepoint * const *end)
>  { }
>  #endif /* CONFIG_TRACEPOINTS */
>  
> @@ -174,12 +171,20 @@ do_trace:								\
>  	{								\
>  	}
>  
> +/*
> + * We have no guarantee that gcc and the linker won't up-align the tracepoint
> + * structures, so we create an array of pointers that will be used for iteration
> + * on the tracepoints.
> + */
>  #define DEFINE_TRACE_FN(name, reg, unreg)				\
>  	static const char __tpstrtab_##name[]				\
>  	__attribute__((section("__tracepoints_strings"))) = #name;	\
>  	struct tracepoint __tracepoint_##name				\
> -	__attribute__((section("__tracepoints"), aligned(32))) =	\
> -		{ __tpstrtab_##name, 0, reg, unreg, NULL }
> +	__attribute__((section("__tracepoints"))) =			\
> +		{ __tpstrtab_##name, 0, reg, unreg, NULL };		\
> +	static struct tracepoint * const __tracepoint_ptr_##name __used	\
> +	__attribute__((section("__tracepoints_ptrs"))) =		\
> +		&__tracepoint_##name;
>  
>  #define DEFINE_TRACE(name)						\
>  	DEFINE_TRACE_FN(name, NULL, NULL);
> diff --git a/kernel/module.c b/kernel/module.c
> index 34e00b7..efa290e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2460,9 +2460,9 @@ static void find_module_sections(struct module *mod, struct load_info *info)
>  #endif
>  
>  #ifdef CONFIG_TRACEPOINTS
> -	mod->tracepoints = section_objs(info, "__tracepoints",
> -					sizeof(*mod->tracepoints),
> -					&mod->num_tracepoints);
> +	mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
> +					     sizeof(*mod->tracepoints_ptrs),
> +					     &mod->num_tracepoints);
>  #endif
>  #ifdef HAVE_JUMP_LABEL
>  	mod->jump_entries = section_objs(info, "__jump_table",
> @@ -3393,7 +3393,7 @@ void module_layout(struct module *mod,
>  		   struct modversion_info *ver,
>  		   struct kernel_param *kp,
>  		   struct kernel_symbol *ks,
> -		   struct tracepoint *tp)
> +		   struct tracepoint * const *tp)
>  {
>  }
>  EXPORT_SYMBOL(module_layout);
> @@ -3407,8 +3407,8 @@ void module_update_tracepoints(void)
>  	mutex_lock(&module_mutex);
>  	list_for_each_entry(mod, &modules, list)
>  		if (!mod->taints)
> -			tracepoint_update_probe_range(mod->tracepoints,
> -				mod->tracepoints + mod->num_tracepoints);
> +			tracepoint_update_probe_range(mod->tracepoints_ptrs,
> +				mod->tracepoints_ptrs + mod->num_tracepoints);
>  	mutex_unlock(&module_mutex);
>  }
>  
> @@ -3432,8 +3432,8 @@ int module_get_iter_tracepoints(struct tracepoint_iter *iter)
>  			else if (iter_mod > iter->module)
>  				iter->tracepoint = NULL;
>  			found = tracepoint_get_iter_range(&iter->tracepoint,
> -				iter_mod->tracepoints,
> -				iter_mod->tracepoints
> +				iter_mod->tracepoints_ptrs,
> +				iter_mod->tracepoints_ptrs
>  					+ iter_mod->num_tracepoints);
>  			if (found) {
>  				iter->module = iter_mod;
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index e95ee7f..68187af 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -27,8 +27,8 @@
>  #include <linux/sched.h>
>  #include <linux/jump_label.h>
>  
> -extern struct tracepoint __start___tracepoints[];
> -extern struct tracepoint __stop___tracepoints[];
> +extern struct tracepoint * const __start___tracepoints_ptrs[];
> +extern struct tracepoint * const __stop___tracepoints_ptrs[];
>  
>  /* Set to 1 to enable tracepoint debug output */
>  static const int tracepoint_debug;
> @@ -298,10 +298,10 @@ static void disable_tracepoint(struct tracepoint *elem)
>   *
>   * Updates the probe callback corresponding to a range of tracepoints.
>   */
> -void
> -tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
> +void tracepoint_update_probe_range(struct tracepoint * const *begin,
> +				   struct tracepoint * const *end)
>  {
> -	struct tracepoint *iter;
> +	struct tracepoint * const *iter;
>  	struct tracepoint_entry *mark_entry;
>  
>  	if (!begin)
> @@ -309,12 +309,12 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>  
>  	mutex_lock(&tracepoints_mutex);
>  	for (iter = begin; iter < end; iter++) {
> -		mark_entry = get_tracepoint(iter->name);
> +		mark_entry = get_tracepoint((*iter)->name);
>  		if (mark_entry) {
> -			set_tracepoint(&mark_entry, iter,
> +			set_tracepoint(&mark_entry, *iter,
>  					!!mark_entry->refcount);
>  		} else {
> -			disable_tracepoint(iter);
> +			disable_tracepoint(*iter);
>  		}
>  	}
>  	mutex_unlock(&tracepoints_mutex);
> @@ -326,8 +326,8 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end)
>  static void tracepoint_update_probes(void)
>  {
>  	/* Core kernel tracepoints */
> -	tracepoint_update_probe_range(__start___tracepoints,
> -		__stop___tracepoints);
> +	tracepoint_update_probe_range(__start___tracepoints_ptrs,
> +		__stop___tracepoints_ptrs);
>  	/* tracepoints in modules. */
>  	module_update_tracepoints();
>  }
> @@ -514,8 +514,8 @@ EXPORT_SYMBOL_GPL(tracepoint_probe_update_all);
>   * Will return the first tracepoint in the range if the input tracepoint is
>   * NULL.
>   */
> -int tracepoint_get_iter_range(struct tracepoint **tracepoint,
> -	struct tracepoint *begin, struct tracepoint *end)
> +int tracepoint_get_iter_range(struct tracepoint * const **tracepoint,
> +	struct tracepoint * const *begin, struct tracepoint * const *end)
>  {
>  	if (!*tracepoint && begin != end) {
>  		*tracepoint = begin;
> @@ -534,7 +534,8 @@ static void tracepoint_get_iter(struct tracepoint_iter *iter)
>  	/* Core kernel tracepoints */
>  	if (!iter->module) {
>  		found = tracepoint_get_iter_range(&iter->tracepoint,
> -				__start___tracepoints, __stop___tracepoints);
> +				__start___tracepoints_ptrs,
> +				__stop___tracepoints_ptrs);
>  		if (found)
>  			goto end;
>  	}
> @@ -585,8 +586,8 @@ int tracepoint_module_notify(struct notifier_block *self,
>  	switch (val) {
>  	case MODULE_STATE_COMING:
>  	case MODULE_STATE_GOING:
> -		tracepoint_update_probe_range(mod->tracepoints,
> -			mod->tracepoints + mod->num_tracepoints);
> +		tracepoint_update_probe_range(mod->tracepoints_ptrs,
> +			mod->tracepoints_ptrs + mod->num_tracepoints);
>  		break;
>  	}
>  	return 0;
> -- 
> 1.7.2.3
> 
> 

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

Re: [PATCH 1/2] tracing: Replace trace_event struct array with pointer array

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> Currently the trace_event structures are placed in the _ftrace_events
> section, and at link time, the linker makes one large array of all
> the trace_event structures. On boot up, this array is read (much like
> the initcall sections) and the events are processed.
> 
> The problem is that there is no guarantee that gcc will place complex
> structures nicely together in an array format. Two structures in the
> same file may be placed awkwardly, because gcc has no clue that they
> are suppose to be in an array.
> 
> A hack was used previous to force the alignment to 4, to pack the
> structures together. But this caused alignment issues with other
> architectures (sparc).
> 
> Instead of packing the structures into an array, the structures' addresses
> are now put into the _ftrace_event section. As pointers are always the
> natural alignment, gcc should always pack them tightly together
> (otherwise initcall, extable, etc would also fail).
> 
> By having the pointers to the structures in the section, we can still
> iterate the trace_events without causing unnecessary alignment problems
> with other architectures, or depending on the current behaviour of
> gcc that will likely change in the future just to tick us kernel developers
> off a little more.
> 
> The _ftrace_event section is also moved into the .init.data section
> as it is now only needed at boot up.
> 
> Suggested-by: David Miller <davem@davemloft.net>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/asm-generic/vmlinux.lds.h |    7 +++----
>  include/linux/module.h            |    2 +-
>  include/linux/syscalls.h          |   10 ++++++----
>  include/trace/ftrace.h            |   24 +++++++++++++-----------
>  kernel/trace/trace_events.c       |   12 ++++++------
>  kernel/trace/trace_export.c       |    6 +++---
>  6 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 6ebb810..f53708b 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -124,7 +124,8 @@
>  #endif
>  
>  #ifdef CONFIG_EVENT_TRACING
> -#define FTRACE_EVENTS()	VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
> +#define FTRACE_EVENTS()	. = ALIGN(8);					\
> +			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
>  			*(_ftrace_events)				\
>  			VMLINUX_SYMBOL(__stop_ftrace_events) = .;
>  #else
> @@ -179,9 +180,6 @@
>  	TRACE_PRINTKS()							\
>  									\
>  	STRUCT_ALIGN();							\
> -	FTRACE_EVENTS()							\
> -									\
> -	STRUCT_ALIGN();							\
>  	TRACE_SYSCALLS()

You seem to have forgotten to fix the __syscalls_metadata table. Do you plan to
do it in another patch ? Its code is pretty much interleaving with the ftrace
code, so it might make sense to do both fixes in one go.

[...]
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index e16610c..3e68366 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -446,14 +446,16 @@ static inline notrace int ftrace_get_offsets_##call(			\
>   *	.reg			= ftrace_event_reg,
>   * };
>   *
> - * static struct ftrace_event_call __used
> - * __attribute__((__aligned__(4)))
> - * __attribute__((section("_ftrace_events"))) event_<call> = {
> + * static struct ftrace_event_call event_<call> = {
>   *	.name			= "<call>",
>   *	.class			= event_class_<template>,
>   *	.event			= &ftrace_event_type_<call>,
>   *	.print_fmt		= print_fmt_<call>,
>   * };
> + * // its only safe to use pointers when doing linker tricks to
> + * // create an array.

Odd comment style.

The rest looks good.

Thanks,

Mathieu

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

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Steven Rostedt]

On Wed, 2011-02-02 at 13:31 -0500, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > 
> > Make the tracepoints more robust, making them solid enough to handle compiler
> > changes by not relying on anything based on compiler-specific behavior with
> > respect to structure alignment. Implement an approach proposed by David Miller:
> > use an array of const pointers to refer to the individual structures, and export
> > this pointer array through the linker script rather than the structures per se.
> > It will consume 32 extra bytes per tracepoint (24 for structure padding and 8
> > for the pointers), but are less likely to break due to compiler changes.
> > 
> > History:
> > 
> > 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.
> > 
> > commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
> > for tracepoint structures by applying both the variable and type attribute to
> > tracepoint structures definitions and declarations. It worked fine with gcc
> > 4.5.1, but broke with gcc 4.4.4 and 4.4.5.
> 
> Small nit: this reference to commit 15e3540ce2159705f18fad6147ffedf04445ad64
> should be changed to a non-commit-id-related explanation, because the commit has
> been rolled back from -tip.

Yeah, I noticed this too, right after I posted it.

> 
> The rest looks good.

OK, I'll rebase by just removing the reverted commit reference.

Thanks,

-- Steve

Re: [PATCH 1/2] tracing: Replace trace_event struct array with pointer array

[Steven Rostedt]

On Wed, 2011-02-02 at 13:42 -0500, Mathieu Desnoyers wrote:

> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index 6ebb810..f53708b 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -124,7 +124,8 @@
> >  #endif
> >  
> >  #ifdef CONFIG_EVENT_TRACING
> > -#define FTRACE_EVENTS()	VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
> > +#define FTRACE_EVENTS()	. = ALIGN(8);					\
> > +			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
> >  			*(_ftrace_events)				\
> >  			VMLINUX_SYMBOL(__stop_ftrace_events) = .;
> >  #else
> > @@ -179,9 +180,6 @@
> >  	TRACE_PRINTKS()							\
> >  									\
> >  	STRUCT_ALIGN();							\
> > -	FTRACE_EVENTS()							\
> > -									\
> > -	STRUCT_ALIGN();							\
> >  	TRACE_SYSCALLS()
> 
> You seem to have forgotten to fix the __syscalls_metadata table. Do you plan to
> do it in another patch ? Its code is pretty much interleaving with the ftrace
> code, so it might make sense to do both fixes in one go.

Thanks for reminding me. No that was going to be a separate patch (it
interleaves with ftrace but is a different entity - developed by two
different people). I was going to do it but got pulled off on a customer
item.

I'll fix that soon too.

> 
> [...]
> > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> > index e16610c..3e68366 100644
> > --- a/include/trace/ftrace.h
> > +++ b/include/trace/ftrace.h
> > @@ -446,14 +446,16 @@ static inline notrace int ftrace_get_offsets_##call(			\
> >   *	.reg			= ftrace_event_reg,
> >   * };
> >   *
> > - * static struct ftrace_event_call __used
> > - * __attribute__((__aligned__(4)))
> > - * __attribute__((section("_ftrace_events"))) event_<call> = {
> > + * static struct ftrace_event_call event_<call> = {
> >   *	.name			= "<call>",
> >   *	.class			= event_class_<template>,
> >   *	.event			= &ftrace_event_type_<call>,
> >   *	.print_fmt		= print_fmt_<call>,
> >   * };
> > + * // its only safe to use pointers when doing linker tricks to
> > + * // create an array.
> 
> Odd comment style.
> 
> The rest looks good.

I knew someone would comment on this comment. Yes, it is an odd comment
stye. The comment is showing "code" that the macro produces. Thus, I
used the '//' to be a comment in the code comment. ;)

-- Steve

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Steven Rostedt]

On Wed, 2011-02-02 at 13:31 -0500, Mathieu Desnoyers wrote:

> > History:
> > 
> > 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.
> > 
> > commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
> > for tracepoint structures by applying both the variable and type attribute to
> > tracepoint structures definitions and declarations. It worked fine with gcc
> > 4.5.1, but broke with gcc 4.4.4 and 4.4.5.
> 
> Small nit: this reference to commit 15e3540ce2159705f18fad6147ffedf04445ad64
> should be changed to a non-commit-id-related explanation, because the commit has
> been rolled back from -tip.

OK, I just did a git commit --amend to fix it. Here's the new text:


    commit 7e066fb870fcd1025ec3ba7bbde5d541094f4ce1 added the aligned(32) type a
    variable attribute to the tracepoint structures to deal with gcc happily
    aligning statically defined structures on 32-byte multiples.
    
    One attempt was to use a 8-byte alignment for tracepoint structures by apply
    both the variable and type attribute to tracepoint structures definitions an
    declarations. It worked fine with gcc 4.5.1, but broke with gcc 4.4.4 and 4.
    
    The reason is that the "aligned" attribute only specify the _minimum_ alignm
    for a structure, leaving both the compiler and the linker free to align on
    larger multiples. Because tracepoint.c expects the structures to be placed a
    array within each section, up-alignment cause NULL-pointer exceptions due to
    extra unexpected padding.

This version has just been pushed. I just changed the wording to leave
out the reference to the reverted commit.

-- Steve

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Wed, 2011-02-02 at 13:31 -0500, Mathieu Desnoyers wrote:
> 
> > > History:
> > > 
> > > 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.
> > > 
> > > commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
> > > for tracepoint structures by applying both the variable and type attribute to
> > > tracepoint structures definitions and declarations. It worked fine with gcc
> > > 4.5.1, but broke with gcc 4.4.4 and 4.4.5.
> > 
> > Small nit: this reference to commit 15e3540ce2159705f18fad6147ffedf04445ad64
> > should be changed to a non-commit-id-related explanation, because the commit has
> > been rolled back from -tip.
> 
> OK, I just did a git commit --amend to fix it. Here's the new text:
> 
> 
>     commit 7e066fb870fcd1025ec3ba7bbde5d541094f4ce1 added the aligned(32) type a
>     variable attribute to the tracepoint structures to deal with gcc happily
>     aligning statically defined structures on 32-byte multiples.
>     
>     One attempt was to use a 8-byte alignment for tracepoint structures by apply
>     both the variable and type attribute to tracepoint structures definitions an
>     declarations. It worked fine with gcc 4.5.1, but broke with gcc 4.4.4 and 4.
>     
>     The reason is that the "aligned" attribute only specify the _minimum_ alignm
>     for a structure, leaving both the compiler and the linker free to align on
>     larger multiples. Because tracepoint.c expects the structures to be placed a
>     array within each section, up-alignment cause NULL-pointer exceptions due to
>     extra unexpected padding.
> 
> This version has just been pushed. I just changed the wording to leave
> out the reference to the reverted commit.

Looks good!

Thanks,

Mathieu

> 
> -- Steve
> 
> 

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

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Thomas Gleixner]

On Wed, 2 Feb 2011, Steven Rostedt wrote:

> On Wed, 2011-02-02 at 13:31 -0500, Mathieu Desnoyers wrote:
> 
> > > History:
> > > 
> > > 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.
> > > 
> > > commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
> > > for tracepoint structures by applying both the variable and type attribute to
> > > tracepoint structures definitions and declarations. It worked fine with gcc
> > > 4.5.1, but broke with gcc 4.4.4 and 4.4.5.
> > 
> > Small nit: this reference to commit 15e3540ce2159705f18fad6147ffedf04445ad64
> > should be changed to a non-commit-id-related explanation, because the commit has
> > been rolled back from -tip.
> 
> OK, I just did a git commit --amend to fix it. Here's the new text:
> 
> 
>     commit 7e066fb870fcd1025ec3ba7bbde5d541094f4ce1 added the aligned(32) type a

Can you please use the common format

    commit 7e066fb8(what ever the subject line of the commit was)

That's way more helpful than a full sha1.

Thanks,

	tglx

Re: [PATCH 1/2] tracing: Replace trace_event struct array with pointer array

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Wed, 02 Feb 2011 13:06:14 -0500

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Currently the trace_event structures are placed in the _ftrace_events
> section, and at link time, the linker makes one large array of all
> the trace_event structures. On boot up, this array is read (much like
> the initcall sections) and the events are processed.
> 
> The problem is that there is no guarantee that gcc will place complex
> structures nicely together in an array format. Two structures in the
> same file may be placed awkwardly, because gcc has no clue that they
> are suppose to be in an array.
> 
> A hack was used previous to force the alignment to 4, to pack the
> structures together. But this caused alignment issues with other
> architectures (sparc).
> 
> Instead of packing the structures into an array, the structures' addresses
> are now put into the _ftrace_event section. As pointers are always the
> natural alignment, gcc should always pack them tightly together
> (otherwise initcall, extable, etc would also fail).
> 
> By having the pointers to the structures in the section, we can still
> iterate the trace_events without causing unnecessary alignment problems
> with other architectures, or depending on the current behaviour of
> gcc that will likely change in the future just to tick us kernel developers
> off a little more.
> 
> The _ftrace_event section is also moved into the .init.data section
> as it is now only needed at boot up.
> 
> Suggested-by: David Miller <davem@davemloft.net>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

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

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Wed, 02 Feb 2011 13:06:15 -0500

> From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> Make the tracepoints more robust, making them solid enough to handle compiler
> changes by not relying on anything based on compiler-specific behavior with
> respect to structure alignment. Implement an approach proposed by David Miller:
> use an array of const pointers to refer to the individual structures, and export
> this pointer array through the linker script rather than the structures per se.
> It will consume 32 extra bytes per tracepoint (24 for structure padding and 8
> for the pointers), but are less likely to break due to compiler changes.
> 
> History:
> 
> 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.
> 
> commit 15e3540ce2159705f18fad6147ffedf04445ad64 tried to use a 8-byte alignment
> for tracepoint structures by applying both the variable and type attribute to
> tracepoint structures definitions and declarations. It worked fine with gcc
> 4.5.1, but broke with gcc 4.4.4 and 4.4.5.
> 
> The reason is that the "aligned" attribute only specify the _minimum_ alignment
> for a structure, leaving both the compiler and the linker free to align on
> larger multiples. Because tracepoint.c expects the structures to be placed as an
> array within each section, up-alignment cause NULL-pointer exceptions due to the
> extra unexpected padding.
> 
> (this patch applies on top of -tip)
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> LKML-Reference: <20110126222622.GA10794@Krystal>
> CC: David Miller <davem@davemloft.net>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

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

Re: [PATCH 2/2] Tracepoints: Fix section alignment using pointer array

[Steven Rostedt]

On Wed, 2011-02-02 at 23:20 +0100, Thomas Gleixner wrote:

> Can you please use the common format
> 
>     commit 7e066fb8(what ever the subject line of the commit was)
> 
> That's way more helpful than a full sha1.
> 

OK, I'll rebase with this update. I'm currently testing the changes for
the syscall metadata (that has the same problem). When I'm done testing,
I'll post a new patch set with David's Acked-bys and this update.

-- Steve

Re: [PATCH 0/2] [GIT PULL][v2.6.38]: tracing: alignment fixes (take 2)

[Steven Rostedt]

On Wed, 2011-02-02 at 13:06 -0500, Steven Rostedt wrote:
> Ingo,
> 
> OK, this time I ran this under x86_64 and x86_32 with gcc-4.5.1
> and gcc-4.4.5. It even found a bug, that I had to fix and retest.
> 

Hi Ingo,

I'm rebasing to fix up some commit logs. I'll post a new patch set
tonight or tomorrow. It will also include a third patch that fixes the
syscall metadata.

-- Steve

Re: [PATCH 1/2] tracing: Replace trace_event struct array with pointer array

[Steven Rostedt]

Hi David,

Did you want to test this patch too. I'm still running it through my
tests, but here's a heads up on it.

-- Steve

commit bc75563dd5b5d8b23f973b3a8ea10fba6f9e93e8
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Wed Feb 2 17:06:09 2011 -0500

    tracing: Replace syscall_meta_data struct array with pointer array
    
    Currently the syscall_meta structures for the syscall tracepoints are
    placed in the __syscall_metadata section, and at link time, the linker
    makes one large array of all these syscall metadata structures. On boot
    up, this array is read (much like the initcall sections) and the syscall
    data is processed.
    
    The problem is that there is no guarantee that gcc will place complex
    structures nicely together in an array format. Two structures in the
    same file may be placed awkwardly, because gcc has no clue that they
    are suppose to be in an array.
    
    A hack was used previous to force the alignment to 4, to pack the
    structures together. But this caused alignment issues with other
    architectures (sparc).
    
    Instead of packing the structures into an array, the structures' addresses
    are now put into the __syscall_metadata section. As pointers are always the
    natural alignment, gcc should always pack them tightly together
    (otherwise initcall, extable, etc would also fail).
    
    By having the pointers to the structures in the section, we can still
    iterate the trace_events without causing unnecessary alignment problems
    with other architectures, or depending on the current behaviour of
    gcc that will likely change in the future just to tick us kernel developers
    off a little more.
    
    The __syscall_metadata section is also moved into the .init.data section
    as it is now only needed at boot up.
    
    Suggested-by: David Miller <davem@davemloft.net>
    Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 57b1b68..fe77e33 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -141,7 +141,8 @@
 #endif
 
 #ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
+#define TRACE_SYSCALLS() . = ALIGN(8);					\
+			 VMLINUX_SYMBOL(__start_syscalls_metadata) = .;	\
 			 *(__syscalls_metadata)				\
 			 VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
 #else
@@ -175,10 +176,7 @@
 	VMLINUX_SYMBOL(__stop___verbose) = .;				\
 	LIKELY_PROFILE()		       				\
 	BRANCH_PROFILE()						\
-	TRACE_PRINTKS()							\
-									\
-	STRUCT_ALIGN();							\
-	TRACE_SYSCALLS()
+	TRACE_PRINTKS()
 
 /*
  * Data section helpers
@@ -483,6 +481,7 @@
 	*(.init.rodata)							\
 	MCOUNT_REC()							\
 	FTRACE_EVENTS()							\
+	TRACE_SYSCALLS()						\
 	DEV_DISCARD(init.rodata)					\
 	CPU_DISCARD(init.rodata)					\
 	MEM_DISCARD(init.rodata)					\
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 45508fe..98664db 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -125,8 +125,7 @@ extern struct trace_event_functions enter_syscall_print_funcs;
 extern struct trace_event_functions exit_syscall_print_funcs;
 
 #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
-	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	static struct syscall_metadata __syscall_meta_##sname;		\
 	static struct ftrace_event_call __used				\
 	  event_enter_##sname = {					\
 		.name                   = "sys_enter"#sname,		\
@@ -140,8 +139,7 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 	__TRACE_EVENT_FLAGS(enter_##sname, TRACE_EVENT_FL_CAP_ANY)
 
 #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
-	static struct syscall_metadata					\
-	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
+	static struct syscall_metadata __syscall_meta_##sname;		\
 	static struct ftrace_event_call __used				\
 	  event_exit_##sname = {					\
 		.name                   = "sys_exit"#sname,		\
@@ -158,8 +156,6 @@ 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)))			\
-	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta_##sname = {				\
 		.name 		= "sys"#sname,			\
 		.nb_args 	= nb,				\
@@ -168,14 +164,15 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 		.enter_event	= &event_enter_##sname,		\
 		.exit_event	= &event_exit_##sname,		\
 		.enter_fields	= LIST_HEAD_INIT(__syscall_meta_##sname.enter_fields), \
-	};
+	};							\
+	static struct syscall_metadata __used			\
+	  __attribute__((section("__syscalls_metadata")))	\
+	 *__p_syscall_meta_##sname = &__syscall_meta_##sname;
 
 #define SYSCALL_DEFINE0(sname)					\
 	SYSCALL_TRACE_ENTER_EVENT(_##sname);			\
 	SYSCALL_TRACE_EXIT_EVENT(_##sname);			\
 	static struct syscall_metadata __used			\
-	  __attribute__((__aligned__(4)))			\
-	  __attribute__((section("__syscalls_metadata")))	\
 	  __syscall_meta__##sname = {				\
 		.name 		= "sys_"#sname,			\
 		.nb_args 	= 0,				\
@@ -183,6 +180,9 @@ extern struct trace_event_functions exit_syscall_print_funcs;
 		.exit_event	= &event_exit__##sname,		\
 		.enter_fields	= LIST_HEAD_INIT(__syscall_meta__##sname.enter_fields), \
 	};							\
+	static struct syscall_metadata __used			\
+	  __attribute__((section("__syscalls_metadata")))	\
+	 *__p_syscall_meta_##sname = &__syscall_meta__##sname;	\
 	asmlinkage long sys_##sname(void)
 #else
 #define SYSCALL_DEFINE0(name)	   asmlinkage long sys_##name(void)
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index b706529..5c9fe08 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -55,20 +55,21 @@ struct ftrace_event_class event_class_syscall_exit = {
 	.raw_init	= init_syscall_trace,
 };
 
-extern unsigned long __start_syscalls_metadata[];
-extern unsigned long __stop_syscalls_metadata[];
+extern struct syscall_metadata *__start_syscalls_metadata[];
+extern struct syscall_metadata *__stop_syscalls_metadata[];
 
 static struct syscall_metadata **syscalls_metadata;
 
-static struct syscall_metadata *find_syscall_meta(unsigned long syscall)
+static __init struct syscall_metadata *
+find_syscall_meta(unsigned long syscall)
 {
-	struct syscall_metadata *start;
-	struct syscall_metadata *stop;
+	struct syscall_metadata **start;
+	struct syscall_metadata **stop;
 	char str[KSYM_SYMBOL_LEN];
 
 
-	start = (struct syscall_metadata *)__start_syscalls_metadata;
-	stop = (struct syscall_metadata *)__stop_syscalls_metadata;
+	start = __start_syscalls_metadata;
+	stop = __stop_syscalls_metadata;
 	kallsyms_lookup(syscall, NULL, NULL, NULL, str);
 
 	for ( ; start < stop; start++) {
@@ -78,8 +79,8 @@ static struct syscall_metadata *find_syscall_meta(unsigned long syscall)
 		 * with "SyS" instead of "sys", leading to an unwanted
 		 * mismatch.
 		 */
-		if (start->name && !strcmp(start->name + 3, str + 3))
-			return start;
+		if ((*start)->name && !strcmp((*start)->name + 3, str + 3))
+			return *start;
 	}
 	return NULL;
 }

Re: [PATCH 1/2] tracing: Replace trace_event struct array with pointer array

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Wed, 02 Feb 2011 19:46:58 -0500

> Did you want to test this patch too. I'm still running it through my
> tests, but here's a heads up on it.

I'll do some testing when you repost the series.  And you can toss
my ACK onto this one right now if you like:

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

Thanks Steven.