Re: [RFC patch 1/3] Kernel Tracepoints
From: Masami Hiramatsu
Date: Thu Jul 03 2008 - 17:49:25 EST
Hi Mathieu,
I couldn't apply this patch against 2.6.26-rc8, and have
some trivial comments.
Mathieu Desnoyers wrote:
> Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.
>
> Allows complete typing verification. No format string required.
>
> TODO : Documentation/tracepoint.txt
>
> Changelog :
> - Use #name ":" #proto as string to identify the tracepoint in the
> tracepoint table. This will make sure not type mismatch happens due to
> connexion of a probe with the wrong type to a tracepoint declared with
> the same name in a different header.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxx>
> CC: 'Peter Zijlstra' <peterz@xxxxxxxxxxxxx>
> CC: "Frank Ch. Eigler" <fche@xxxxxxxxxx>
> CC: 'Ingo Molnar' <mingo@xxxxxxx>
> CC: 'Hideo AOKI' <haoki@xxxxxxxxxx>
> CC: Takashi Nishiie <t-nishiie@xxxxxxxxxxxxxxxxxx>
> CC: 'Steven Rostedt' <rostedt@xxxxxxxxxxx>
> CC: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> include/asm-generic/vmlinux.lds.h | 6
> include/linux/module.h | 17 +
> include/linux/tracepoint.h | 139 ++++++++++
> init/Kconfig | 16 +
> kernel/Makefile | 1
> kernel/module.c | 66 ++++-
> kernel/tracepoint.c | 498 ++++++++++++++++++++++++++++++++++++++
> 7 files changed, 741 insertions(+), 2 deletions(-)
>
> Index: linux-2.6-lttng/init/Kconfig
> ===================================================================
> --- linux-2.6-lttng.orig/init/Kconfig 2008-07-03 11:47:15.000000000 -0400
> +++ linux-2.6-lttng/init/Kconfig 2008-07-03 11:49:54.000000000 -0400
> @@ -782,12 +782,28 @@ config PROFILING
> Say Y here to enable the extended profiling support mechanisms used
> by profilers such as OProfile.
>
> +config TRACEPOINTS
> + bool "Activate tracepoints"
> + default y
> + help
> + Place an empty function call at each tracepoint site. Can be
> + dynamically changed for a probe function.
> +
> config MARKERS
> bool "Activate markers"
> help
> Place an empty function call at each marker site. Can be
> dynamically changed for a probe function.
>
> +config TRACEPROBES
> + tristate "Compile generic tracing probes"
> + depends on MARKERS
> + default y
> + help
> + Compile generic tracing probes, which connect to the tracepoints when
> + loaded and format the information collected by the tracepoints with
> + the Markers.
> +
> source "arch/Kconfig"
might this part better go to PATCH 3/3?
[...]
> +
> +
> +#define TPPROTO(args...) args
> +#define TPARGS(args...) args
> +
> +#ifdef CONFIG_TRACEPOINTS
> +
> +/*
> + * Note : the empty asm volatile with read constraint is used here instead of a
> + * "used" attribute to fix a gcc 4.1.x bug.
There seems no empty asm...
> + * Make sure the alignment of the structure in the __tracepoints section will
> + * not add unwanted padding between the beginning of the section and the
> + * structure. Force alignment to the same alignment as the section start.
> + */
> +#define DEFINE_TRACE(name, proto, args) \
> + static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> + { \
> + int i; \
> + struct tracepoint_probe_closure *multi; \
> + preempt_disable(); \
> + multi = tp->multi; \
> + smp_read_barrier_depends(); \
> + if (multi) { \
> + for (i = 0; multi[i].func; i++) { \
> + ((void(*)(void *private_data, proto)) \
> + (multi[i].func))(multi[i].probe_private, args);\
> + } \
> + } \
> + preempt_enable(); \
> + } \
> + static inline void trace_##name(proto) \
> + { \
> + static const char __tpstrtab_##name[] \
> + __attribute__((section("__tracepoints_strings"))) \
> + = #name ":" #proto; \
> + static struct tracepoint __tracepoint_##name \
> + __attribute__((section("__tracepoints"), aligned(8))) = \
> + { __tpstrtab_##name, 0, NULL }; \
> + if (unlikely(__tracepoint_##name.state)) \
> + _do_trace_##name(&__tracepoint_##name, args); \
> + } \
> + static inline int register_trace_##name( \
> + void (*probe)(void *private_data, proto), \
> + void *private_data) \
> + { \
> + return tracepoint_probe_register(#name ":" #proto, \
> + (void *)probe, private_data); \
> + } \
> + static inline void unregister_trace_##name( \
> + void (*probe)(void *private_data, proto), \
> + void *private_data) \
> + { \
> + tracepoint_probe_unregister(#name ":" #proto, \
> + (void *)probe, private_data); \
> + }
> +
As we are discussing on another thread, this macro can't handle
non-argument tracepoint.
[...]
> Index: linux-2.6-lttng/kernel/tracepoint.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-lttng/kernel/tracepoint.c 2008-07-03 11:54:30.000000000 -0400
> @@ -0,0 +1,498 @@
> +/*
> + * Copyright (C) 2007 Mathieu Desnoyers
trivial: it should be 2008? :-)
[...]
> Index: linux-2.6-lttng/kernel/module.c
> ===================================================================
> --- linux-2.6-lttng.orig/kernel/module.c 2008-07-03 11:49:54.000000000 -0400
> +++ linux-2.6-lttng/kernel/module.c 2008-07-03 11:54:01.000000000 -0400
> @@ -46,6 +46,7 @@
> #include <asm/cacheflush.h>
> #include <linux/license.h>
> #include <asm/sections.h>
> +#include <linux/tracepoint.h>
>
> #if 0
> #define DEBUGP printk
> @@ -1770,6 +1771,8 @@ static struct module *load_module(void _
> unsigned int unusedgplcrcindex;
> unsigned int markersindex;
> unsigned int markersstringsindex;
> + unsigned int tracepointsindex;
> + unsigned int tracepointsstringsindex;
> struct module *mod;
> long err = 0;
> void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
> @@ -2049,6 +2052,9 @@ static struct module *load_module(void _
> markersindex = find_sec(hdr, sechdrs, secstrings, "__markers");
> markersstringsindex = find_sec(hdr, sechdrs, secstrings,
> "__markers_strings");
> + tracepointsindex = find_sec(hdr, sechdrs, secstrings, "__tracepoints");
> + tracepointsstringsindex = find_sec(hdr, sechdrs, secstrings,
> + "__tracepoints_strings");
>
> /* Now do relocations. */
> for (i = 1; i < hdr->e_shnum; i++) {
> @@ -2076,6 +2082,12 @@ static struct module *load_module(void _
> mod->num_markers =
> sechdrs[markersindex].sh_size / sizeof(*mod->markers);
> #endif
> +#ifdef CONFIG_TRACEPOINTS
> + mod->tracepoints = (void *)sechdrs[tracepointsindex].sh_addr;
> + mod->num_tracepoints =
> + sechdrs[tracepointsindex].sh_size / sizeof(*mod->tracepoints);
> +#endif
> +
>
> /* Find duplicate symbols */
> err = verify_export_symbols(mod);
> @@ -2094,11 +2106,16 @@ static struct module *load_module(void _
>
> add_kallsyms(mod, sechdrs, symindex, strindex, secstrings);
>
> + if (!(mod->taints & TAINT_FORCED_MODULE)) {
> #ifdef CONFIG_MARKERS
> - if (!(mod->taints & TAINT_FORCED_MODULE))
> marker_update_probe_range(mod->markers,
> mod->markers + mod->num_markers);
Here, this hunk was rejected, because "Markers Support for
Proprierary Modules" patch doesn't merged yet.
> #endif
> +#ifdef CONFIG_TRACEPOINTS
> + tracepoint_update_probe_range(mod->tracepoints,
> + mod->tracepoints + mod->num_tracepoints);
> +#endif
> + }
> err = module_finalize(hdr, sechdrs, mod);
> if (err < 0)
> goto cleanup;
Thank you,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: mhiramat@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/