lkml.org 
[lkml]   [2014]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v10 1/1] Tracepoint: register/unregister struct tracepoint
    ----- Original Message -----
    > From: "Steven Rostedt" <rostedt@goodmis.org>
    > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
    > Cc: linux-kernel@vger.kernel.org, "Ingo Molnar" <mingo@kernel.org>, "Frederic Weisbecker" <fweisbec@gmail.com>,
    > "Andrew Morton" <akpm@linux-foundation.org>, "Frank Ch. Eigler" <fche@redhat.com>, "Johannes Berg"
    > <johannes.berg@intel.com>
    > Sent: Tuesday, April 8, 2014 1:56:00 PM
    > Subject: Re: [PATCH v10 1/1] Tracepoint: register/unregister struct tracepoint
    >
    > On Fri, 4 Apr 2014 10:02:34 -0400
    > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
    >
    > > Register/unregister tracepoint probes with struct tracepoint pointer
    > > rather than tracepoint name.
    > >
    > > This change, which vastly simplifies tracepoint.c, has been proposed by
    > > Steven Rostedt. It also removes 8.8kB (mostly of text) to the vmlinux
    > > size.
    > >
    > > >From this point on, the tracers need to pass a struct tracepoint pointer
    > > to probe register/unregister. A probe can now only be connected to a
    > > tracepoint that exists. Moreover, tracers are responsible for
    > > unregistering the probe before the module containing its associated
    > > tracepoint is unloaded.
    > >
    > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    > > CC: Steven Rostedt <rostedt@goodmis.org>
    > > CC: Ingo Molnar <mingo@kernel.org>
    > > CC: Frederic Weisbecker <fweisbec@gmail.com>
    > > CC: Andrew Morton <akpm@linux-foundation.org>
    > > CC: Frank Ch. Eigler <fche@redhat.com>
    > > CC: Johannes Berg <johannes.berg@intel.com>
    > > ---
    > > include/linux/ftrace_event.h | 22 +-
    > > include/linux/tracepoint.h | 41 +--
    > > include/trace/ftrace.h | 9 +-
    > > kernel/trace/trace_events.c | 51 ++--
    > > kernel/trace/trace_events_trigger.c | 2 +-
    > > kernel/trace/trace_kprobe.c | 21 +-
    > > kernel/trace/trace_output.c | 2 +-
    > > kernel/trace/trace_uprobe.c | 20 +-
    > > kernel/tracepoint.c | 509
    > > +++++++++++++++--------------------
    > > 9 files changed, 328 insertions(+), 349 deletions(-)
    > >
    > > diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
    > > index cdc3011..75e64e8 100644
    > > --- a/include/linux/ftrace_event.h
    > > +++ b/include/linux/ftrace_event.h
    > > @@ -7,6 +7,7 @@
    > > #include <linux/percpu.h>
    > > #include <linux/hardirq.h>
    > > #include <linux/perf_event.h>
    > > +#include <linux/tracepoint.h>
    > >
    > > struct trace_array;
    > > struct trace_buffer;
    > > @@ -232,6 +233,7 @@ enum {
    > > TRACE_EVENT_FL_IGNORE_ENABLE_BIT,
    > > TRACE_EVENT_FL_WAS_ENABLED_BIT,
    > > TRACE_EVENT_FL_USE_CALL_FILTER_BIT,
    > > + TRACE_EVENT_FL_TRACEPOINT_BIT,
    > > };
    > >
    > > /*
    > > @@ -244,6 +246,7 @@ enum {
    > > * (used for module unloading, if a module event is
    > > enabled,
    > > * it is best to clear the buffers that used it).
    > > * USE_CALL_FILTER - For ftrace internal events, don't use file filter
    > > + * TRACEPOINT - Event is a tracepoint
    > > */
    > > enum {
    > > TRACE_EVENT_FL_FILTERED = (1 << TRACE_EVENT_FL_FILTERED_BIT),
    > > @@ -252,12 +255,17 @@ enum {
    > > TRACE_EVENT_FL_IGNORE_ENABLE = (1 << TRACE_EVENT_FL_IGNORE_ENABLE_BIT),
    > > TRACE_EVENT_FL_WAS_ENABLED = (1 << TRACE_EVENT_FL_WAS_ENABLED_BIT),
    > > TRACE_EVENT_FL_USE_CALL_FILTER = (1 <<
    > > TRACE_EVENT_FL_USE_CALL_FILTER_BIT),
    > > + TRACE_EVENT_FL_TRACEPOINT = (1 << TRACE_EVENT_FL_TRACEPOINT_BIT),
    > > };
    > >
    > > struct ftrace_event_call {
    > > struct list_head list;
    > > struct ftrace_event_class *class;
    > > - char *name;
    > > + union {
    > > + char *name;
    > > + /* Set TRACE_EVENT_FL_TRACEPOINT flag when using "tp" */
    > > + struct tracepoint *tp;
    > > + };
    > > struct trace_event event;
    > > const char *print_fmt;
    > > struct event_filter *filter;
    > > @@ -271,6 +279,7 @@ struct ftrace_event_call {
    > > * bit 3: ftrace internal event (do not enable)
    > > * bit 4: Event was enabled by module
    > > * bit 5: use call filter rather than file filter
    > > + * bit 6: Event is a tracepoint
    > > */
    > > int flags; /* static flags of different events */
    > >
    > > @@ -283,6 +292,15 @@ struct ftrace_event_call {
    > > #endif
    > > };
    > >
    > > +static inline const char *
    > > +ftrace_event_get_name(struct ftrace_event_call *call)
    >
    > I was thinking about this, and it really should be called
    > "ftrace_event_name()", the "get" implies that there should be an
    > associated "put".

    OK. I'll change it.

    >
    > > +{
    > > + if (call->flags & TRACE_EVENT_FL_TRACEPOINT)
    > > + return call->tp ? call->tp->name : NULL;
    > > + else
    > > + return call->name;
    > > +}
    >
    > Also, you missed a bunch of conversions in trace_event.c file. Do a
    > search for '->name' and you'll find them. The ones that shouldn't be
    > converted are rather obvious (system, field, mod), but you'll see
    > others that were missed. I'm a bit nervous about what other ones may be
    > missed as well.

    Got them. I noticed that I forgot to turn on CONFIG_DYNAMIC_FTRACE when
    I did the original swap from name to _name to find the sites to update.

    I did test more recently with a config that has all the FTRACE and PERF
    features enabled, but that was after I swapped back from _name to name,
    so I did not catch those two sites.

    I just swapped the field name to _name again, and did the build with
    all FTRACE and PERF config options enabled to be certain I did not miss
    any. Just to be extra sure, I'm doing a make allyesconfig too.

    Then, I'm changing _name back to name, and I'll prepare this as v11.
    Is that OK with you ?

    Thanks,

    Mathieu


    >
    > -- Steve
    >

    --
    Mathieu Desnoyers
    EfficiOS Inc.
    http://www.efficios.com


    \
     
     \ /
      Last update: 2014-04-08 21:41    [W:3.664 / U:1.300 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site