linux-kernel - Re: [PATCH v10 1/1] Tracepoint: register/unregister struct tracepoint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 8 Apr 2014 13:56:00 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Johannes Berg <johannes.berg@...el.com>
Subject: Re: [PATCH v10 1/1] Tracepoint: register/unregister struct
 tracepoint

On Fri,  4 Apr 2014 10:02:34 -0400
Mathieu Desnoyers <mathieu.desnoyers@...icios.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@...icios.com>
> CC: Steven Rostedt <rostedt@...dmis.org>
> CC: Ingo Molnar <mingo@...nel.org>
> CC: Frederic Weisbecker <fweisbec@...il.com>
> CC: Andrew Morton <akpm@...ux-foundation.org>
> CC: Frank Ch. Eigler <fche@...hat.com>
> CC: Johannes Berg <johannes.berg@...el.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".

> +{
> +	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.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/