lkml.org 
[lkml]   [2011]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] tracing: Replace trace_event struct array with pointer array
* 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


\
 
 \ /
  Last update: 2011-02-02 19:45    [W:0.053 / U:1.172 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site