[RFC] tracing: Enable tracepoints via module parameters
diff mbox series

Message ID 1299622684.20306.77.camel@gandalf.stny.rr.com
State New, archived
Headers show
Series
  • [RFC] tracing: Enable tracepoints via module parameters
Related show

Commit Message

Steven Rostedt March 8, 2011, 10:18 p.m. UTC
A few months ago it was suggested to have a way to enable tracepoints in
a module when it is loaded. I tried various methods, but this one seems
to be the least intrusive. In fact, it requires no modification to the
module code.

The trace event now adds its own MODULE_INFO() and kernel_param_ops that
and links the information about a tracepoint into the module's __param
section. A module can be loaded with a tracepoint active by adding
trace_<tracepoint>=1 as one of the parameters.

The following patch is in:

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

    branch: rfc/trace


Steven Rostedt (1):
      tracing: Enable tracepoints via module parameters

----
 include/linux/ftrace_event.h |    4 +++
 include/trace/ftrace.h       |   22 ++++++++++++++++-
 kernel/trace/trace_events.c  |   52 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)
---------------------------
commit b514aa1b59148994a47086cdd46db7f994b4789e
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Tue Mar 8 17:06:14 2011 -0500

    tracing: Enable tracepoints via module parameters
    
    Add the tracepoints within the module to the module info section
    and allow the tracepoints to be enabled when the module is loaded.
    
    For example:
    
    [root]# modinfo samples/trace_events/trace-events-sample.ko
    filename:       samples/trace_events/trace-events-sample.ko
    license:        GPL
    description:    trace-events-sample
    author:         Steven Rostedt
    tracepoint:     foo_bar
    srcversion:     F6AC4B8D911A5C5B9CCDD7B
    depends:
    vermagic:       2.6.38-rc5+ SMP preempt mod_unload
    
    Notice that the trace-events-sample tracepoint "foo_bar" shows
    up in the modinfo. When loading the module with:
    
     # insmod trace-events-sample.ko trace_foo_bar=1
    
    The tracepoint "foo_bar" will be enabled.
    
    Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
    Cc: Chris Wilson <chris@chris-wilson.co.uk>
    Cc: Rusty Russell <rusty@rustycorp.com.au>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>



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

Comments

Steven Rostedt March 8, 2011, 10:42 p.m. UTC | #1
On Tue, 2011-03-08 at 17:18 -0500, Steven Rostedt wrote:
> A few months ago it was suggested to have a way to enable tracepoints in
> a module when it is loaded. I tried various methods, but this one seems
> to be the least intrusive. In fact, it requires no modification to the
> module code.

I forgot to include the link to where this was brought up before:

https://lkml.org/lkml/2010/11/9/79

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers March 8, 2011, 11:22 p.m. UTC | #2
* Steven Rostedt (rostedt@goodmis.org) wrote:
> A few months ago it was suggested to have a way to enable tracepoints in
> a module when it is loaded. I tried various methods, but this one seems
> to be the least intrusive. In fact, it requires no modification to the
> module code.
> 
> The trace event now adds its own MODULE_INFO() and kernel_param_ops that
> and links the information about a tracepoint into the module's __param
> section. A module can be loaded with a tracepoint active by adding
> trace_<tracepoint>=1 as one of the parameters.

Hi Steven,

Can you walk me through the expected sequence someone wanting to enable a few
specific module tracepoints would have to go through ? I'm thinking here about
the context of a distro which has on-demand module loading. The scenario I am
thinking about is a distro specifying a basic set of tracepoints to enable in a
"standard catch-all tracing configuration", which includes some tracepoints in
yet-unloaded modules. I'm trying to figure out what the end user experience will
look like if we go for the solution you propose here.

Thanks,

Mathieu

> 
> The following patch is in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: rfc/trace
> 
> 
> Steven Rostedt (1):
>       tracing: Enable tracepoints via module parameters
> 
> ----
>  include/linux/ftrace_event.h |    4 +++
>  include/trace/ftrace.h       |   22 ++++++++++++++++-
>  kernel/trace/trace_events.c  |   52 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 2 deletions(-)
> ---------------------------
> commit b514aa1b59148994a47086cdd46db7f994b4789e
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Tue Mar 8 17:06:14 2011 -0500
> 
>     tracing: Enable tracepoints via module parameters
>     
>     Add the tracepoints within the module to the module info section
>     and allow the tracepoints to be enabled when the module is loaded.
>     
>     For example:
>     
>     [root]# modinfo samples/trace_events/trace-events-sample.ko
>     filename:       samples/trace_events/trace-events-sample.ko
>     license:        GPL
>     description:    trace-events-sample
>     author:         Steven Rostedt
>     tracepoint:     foo_bar
>     srcversion:     F6AC4B8D911A5C5B9CCDD7B
>     depends:
>     vermagic:       2.6.38-rc5+ SMP preempt mod_unload
>     
>     Notice that the trace-events-sample tracepoint "foo_bar" shows
>     up in the modinfo. When loading the module with:
>     
>      # insmod trace-events-sample.ko trace_foo_bar=1
>     
>     The tracepoint "foo_bar" will be enabled.
>     
>     Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>     Cc: Chris Wilson <chris@chris-wilson.co.uk>
>     Cc: Rusty Russell <rusty@rustycorp.com.au>
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 47e3997..b672889 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -124,6 +124,8 @@ void tracing_record_cmdline(struct task_struct *tsk);
>  
>  struct event_filter;
>  
> +extern struct kernel_param_ops ftrace_mod_ops;
> +
>  enum trace_reg {
>  	TRACE_REG_REGISTER,
>  	TRACE_REG_UNREGISTER,
> @@ -155,6 +157,7 @@ enum {
>  	TRACE_EVENT_FL_FILTERED_BIT,
>  	TRACE_EVENT_FL_RECORDED_CMD_BIT,
>  	TRACE_EVENT_FL_CAP_ANY_BIT,
> +	TRACE_EVENT_FL_MOD_ENABLE_BIT,
>  };
>  
>  enum {
> @@ -162,6 +165,7 @@ enum {
>  	TRACE_EVENT_FL_FILTERED		= (1 << TRACE_EVENT_FL_FILTERED_BIT),
>  	TRACE_EVENT_FL_RECORDED_CMD	= (1 << TRACE_EVENT_FL_RECORDED_CMD_BIT),
>  	TRACE_EVENT_FL_CAP_ANY		= (1 << TRACE_EVENT_FL_CAP_ANY_BIT),
> +	TRACE_EVENT_FL_MOD_ENABLE	= (1 << TRACE_EVENT_FL_MOD_ENABLE_BIT),
>  };
>  
>  struct ftrace_event_call {
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 3e68366..3d4a6ee 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -561,6 +561,22 @@ static inline void ftrace_test_probe_##call(void)			\
>  #undef __get_dynamic_array
>  #undef __get_str
>  
> +/*
> + * Add ftrace trace points in modules to be set by module
> + * parameters. This adds "trace_##call" as a module parameter.
> + * The user could enable trace points on module load with:
> + *  trace_##call=1 as a module parameter.
> + */
> +#undef ftrace_mod_params
> +#ifdef MODULE
> +#define ftrace_mod_params(call)				\
> +	module_param_cb(trace_##call, &ftrace_mod_ops,	\
> +			&event_##call, 0644);		\
> +	MODULE_INFO(tracepoint, #call)
> +#else
> +#define ftrace_mod_params(call)
> +#endif
> +
>  #undef TP_printk
>  #define TP_printk(fmt, args...) "\"" fmt "\", "  __stringify(args)
>  
> @@ -588,7 +604,8 @@ static struct ftrace_event_call __used event_##call = {			\
>  	.print_fmt		= print_fmt_##template,			\
>  };									\
>  static struct ftrace_event_call __used					\
> -__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
> +__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call;\
> +ftrace_mod_params(call)
>  
>  #undef DEFINE_EVENT_PRINT
>  #define DEFINE_EVENT_PRINT(template, call, proto, args, print)		\
> @@ -602,7 +619,8 @@ static struct ftrace_event_call __used event_##call = {			\
>  	.print_fmt		= print_fmt_##call,			\
>  };									\
>  static struct ftrace_event_call __used					\
> -__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
> +__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call; \
> +ftrace_mod_params(call)
>  
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>  
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 5f499e0..5223751 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1285,6 +1285,7 @@ static void trace_module_add_events(struct module *mod)
>  {
>  	struct ftrace_module_file_ops *file_ops = NULL;
>  	struct ftrace_event_call **call, **start, **end;
> +	int ret;
>  
>  	start = mod->trace_events;
>  	end = mod->trace_events + mod->num_trace_events;
> @@ -1300,6 +1301,14 @@ static void trace_module_add_events(struct module *mod)
>  		__trace_add_event_call(*call, mod,
>  				       &file_ops->id, &file_ops->enable,
>  				       &file_ops->filter, &file_ops->format);
> +		/* If the module tracepoint parameter was set */
> +		if ((*call)->flags & TRACE_EVENT_FL_MOD_ENABLE) {
> +			(*call)->flags = 0;
> +			ret = ftrace_event_enable_disable(*call, 1);
> +			if (ret < 0)
> +				pr_warning("unable to enable tracepoint %s",
> +					   (*call)->name);
> +		}
>  	}
>  }
>  
> @@ -1457,6 +1466,49 @@ static __init int event_trace_init(void)
>  }
>  fs_initcall(event_trace_init);
>  
> +/* Allow modules to load with enabled trace points */
> +int ftrace_mod_param_set(const char *val, const struct kernel_param *kp)
> +{
> +	struct ftrace_event_call *call = kp->arg;
> +
> +	/* This is set like param_set_bool() */
> +
> +	/* No equals means "set"... */
> +	if (!val)
> +		val = "1";
> +
> +	/* One of =[yYnN01] */
> +	switch (val[0]) {
> +	case 'y': case 'Y': case '1':
> +		break;
> +	case 'n': case 'N': case '0':
> +		/* Do nothing */
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Set flag to tell ftrace to enable this event on init */
> +	call->flags = TRACE_EVENT_FL_MOD_ENABLE;
> +
> +	return 0;
> +}
> +
> +int ftrace_mod_param_get(char *buffer, const struct kernel_param *kp)
> +{
> +	struct ftrace_event_call *call = kp->arg;
> +
> +	return sprintf(buffer, "%d",
> +		   !!(call->flags &
> +		     (TRACE_EVENT_FL_MOD_ENABLE | TRACE_EVENT_FL_ENABLED)));
> +}
> +
> +struct kernel_param_ops ftrace_mod_ops = {
> +	.set = ftrace_mod_param_set,
> +	.get = ftrace_mod_param_get,
> +};
> +EXPORT_SYMBOL(ftrace_mod_ops);
> +
>  #ifdef CONFIG_FTRACE_STARTUP_TEST
>  
>  static DEFINE_SPINLOCK(test_spinlock);
> 
>
Steven Rostedt March 8, 2011, 11:32 p.m. UTC | #3
On Tue, 2011-03-08 at 18:22 -0500, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > A few months ago it was suggested to have a way to enable tracepoints in
> > a module when it is loaded. I tried various methods, but this one seems
> > to be the least intrusive. In fact, it requires no modification to the
> > module code.
> > 
> > The trace event now adds its own MODULE_INFO() and kernel_param_ops that
> > and links the information about a tracepoint into the module's __param
> > section. A module can be loaded with a tracepoint active by adding
> > trace_<tracepoint>=1 as one of the parameters.
> 
> Hi Steven,
> 
> Can you walk me through the expected sequence someone wanting to enable a few
> specific module tracepoints would have to go through ? I'm thinking here about
> the context of a distro which has on-demand module loading. The scenario I am
> thinking about is a distro specifying a basic set of tracepoints to enable in a
> "standard catch-all tracing configuration", which includes some tracepoints in
> yet-unloaded modules. I'm trying to figure out what the end user experience will
> look like if we go for the solution you propose here.
> 

You would add it like any other module parameter.

Just update it in your /etc/modprobe.d/ directory.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers March 9, 2011, 12:07 a.m. UTC | #4
* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Tue, 2011-03-08 at 18:22 -0500, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > A few months ago it was suggested to have a way to enable tracepoints in
> > > a module when it is loaded. I tried various methods, but this one seems
> > > to be the least intrusive. In fact, it requires no modification to the
> > > module code.
> > > 
> > > The trace event now adds its own MODULE_INFO() and kernel_param_ops that
> > > and links the information about a tracepoint into the module's __param
> > > section. A module can be loaded with a tracepoint active by adding
> > > trace_<tracepoint>=1 as one of the parameters.
> > 
> > Hi Steven,
> > 
> > Can you walk me through the expected sequence someone wanting to enable a few
> > specific module tracepoints would have to go through ? I'm thinking here about
> > the context of a distro which has on-demand module loading. The scenario I am
> > thinking about is a distro specifying a basic set of tracepoints to enable in a
> > "standard catch-all tracing configuration", which includes some tracepoints in
> > yet-unloaded modules. I'm trying to figure out what the end user experience will
> > look like if we go for the solution you propose here.
> > 
> 
> You would add it like any other module parameter.
> 
> Just update it in your /etc/modprobe.d/ directory.

So what you are saying here is that modifying /etc/modprobe.d/ is the actual
interface you propose presenting to the end-users to control their tracepoints ?

Maybe I am missing something, but this interface seems to lack the layer of
finish we might want to put into a user-visible API. I don't really see how
distributions can hope to automate any of this for their end-user without making
a mess of the /etc/modprobe.d/ they ship with.

Thanks,

Mathieu
Steven Rostedt March 9, 2011, 12:14 a.m. UTC | #5
On Tue, 2011-03-08 at 19:07 -0500, Mathieu Desnoyers wrote:

> So what you are saying here is that modifying /etc/modprobe.d/ is the actual
> interface you propose presenting to the end-users to control their tracepoints ?

If you want to have them enabled on boot, sure.

> Maybe I am missing something, but this interface seems to lack the layer of
> finish we might want to put into a user-visible API. I don't really see how
> distributions can hope to automate any of this for their end-user without making
> a mess of the /etc/modprobe.d/ they ship with.

What distros enable tracepoints by default?

If you want to enable a tracepoint on module load simply do:

 modprobe mymod trace_my_tracepoint=1

Otherwise modify your modprobe.d directory. This is the way users have
been doing module parameters for years.

That's pretty simple to me.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers March 9, 2011, 12:29 a.m. UTC | #6
* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Tue, 2011-03-08 at 19:07 -0500, Mathieu Desnoyers wrote:
> 
> > So what you are saying here is that modifying /etc/modprobe.d/ is the actual
> > interface you propose presenting to the end-users to control their tracepoints ?
> 
> If you want to have them enabled on boot, sure.

No, I'm not talking about enabling tracepoints on boot here. Let's think about a
basic use-case: a distro packages a tracer, and provides a "default" set of
tracepoints that can be enabled when tracing is needed. Therefore, these
tracepoints are not enabled at boot -- they are rather enabled when tracing
starts. Some of these tracepoints are in dynamically loaded modules. Some of
these modules are automatically loaded by the distro (e.g. when a USB device is
plugged in).

The specific module tracepoints should therefore only be enabled when both of
the following conditions are true:

A - the end-user want to trace
B - the module is loaded

But it looks like hooking on modinfo will only let you unconditionally enable
the module tracepoints during normal system operations (even when tracing is
off), or not at all unless you previously load the module (which does not fit
with the reality of distributions automagically loading modules on demand while
tracing runs).

> 
> > Maybe I am missing something, but this interface seems to lack the layer of
> > finish we might want to put into a user-visible API. I don't really see how
> > distributions can hope to automate any of this for their end-user without making
> > a mess of the /etc/modprobe.d/ they ship with.
> 
> What distros enable tracepoints by default?

Do you mean enable as having a probe connected, or just CONFIG_TRACEPOINTS=y ?

> 
> If you want to enable a tracepoint on module load simply do:
> 
>  modprobe mymod trace_my_tracepoint=1
> 
> Otherwise modify your modprobe.d directory. This is the way users have
> been doing module parameters for years.
> 
> That's pretty simple to me.

Everything is always so much easier when your end-user target is yourself.
What are users for anyway ? :-P

Mathieu
Steven Rostedt March 9, 2011, 12:52 a.m. UTC | #7
On Tue, 2011-03-08 at 19:29 -0500, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > On Tue, 2011-03-08 at 19:07 -0500, Mathieu Desnoyers wrote:
> > 
> > > So what you are saying here is that modifying /etc/modprobe.d/ is the actual
> > > interface you propose presenting to the end-users to control their tracepoints ?
> > 
> > If you want to have them enabled on boot, sure.
> 
> No, I'm not talking about enabling tracepoints on boot here. Let's think about a
> basic use-case: a distro packages a tracer, and provides a "default" set of
> tracepoints that can be enabled when tracing is needed. Therefore, these
> tracepoints are not enabled at boot -- they are rather enabled when tracing
> starts. Some of these tracepoints are in dynamically loaded modules. Some of
> these modules are automatically loaded by the distro (e.g. when a USB device is
> plugged in).

What distro are we talking about? What distro provides a "default" set
of tracepoints?


> 
> The specific module tracepoints should therefore only be enabled when both of
> the following conditions are true:
> 
> A - the end-user want to trace
> B - the module is loaded
> 
> But it looks like hooking on modinfo will only let you unconditionally enable
> the module tracepoints during normal system operations (even when tracing is
> off), or not at all unless you previously load the module (which does not fit
> with the reality of distributions automagically loading modules on demand while
> tracing runs).

Lets keep it simple please. Right now my proposal does more than what we
currently have. Perhaps we could change the enabling to only enable if
"tracing is on", or some /proc/sys/kernel/X flag is set.


> > 
> > > Maybe I am missing something, but this interface seems to lack the layer of
> > > finish we might want to put into a user-visible API. I don't really see how
> > > distributions can hope to automate any of this for their end-user without making
> > > a mess of the /etc/modprobe.d/ they ship with.
> > 
> > What distros enable tracepoints by default?
> 
> Do you mean enable as having a probe connected, or just CONFIG_TRACEPOINTS=y ?

I mean having the probe connected as distros already have
CONFIG_TRACEPOINTS on. What was your meaning when you said "distro
specifying a basic set of tracepoints to enable"?


> 
> > 
> > If you want to enable a tracepoint on module load simply do:
> > 
> >  modprobe mymod trace_my_tracepoint=1
> > 
> > Otherwise modify your modprobe.d directory. This is the way users have
> > been doing module parameters for years.
> > 
> > That's pretty simple to me.
> 
> Everything is always so much easier when your end-user target is yourself.
> What are users for anyway ? :-P

Users are for testing code ;)

But that's a good question. As I wrote this because I'm purging my inbox
and came across Yuanhan Liu's patch set. I'm curios to what Yuanhan's
motivation for this change was.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers March 9, 2011, 1:17 a.m. UTC | #8
* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Tue, 2011-03-08 at 19:29 -0500, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > On Tue, 2011-03-08 at 19:07 -0500, Mathieu Desnoyers wrote:
> > > 
> > > > So what you are saying here is that modifying /etc/modprobe.d/ is the actual
> > > > interface you propose presenting to the end-users to control their tracepoints ?
> > > 
> > > If you want to have them enabled on boot, sure.
> > 
> > No, I'm not talking about enabling tracepoints on boot here. Let's think about a
> > basic use-case: a distro packages a tracer, and provides a "default" set of
> > tracepoints that can be enabled when tracing is needed. Therefore, these
> > tracepoints are not enabled at boot -- they are rather enabled when tracing
> > starts. Some of these tracepoints are in dynamically loaded modules. Some of
> > these modules are automatically loaded by the distro (e.g. when a USB device is
> > plugged in).
> 
> What distro are we talking about? What distro provides a "default" set
> of tracepoints?

I'm afraid I cannot say, at this point, which distro I am refering to, because
that would be a little forward of me to push news before official feature
announcements.

And about the "default" tracepoints, let's mainly think about tracepoints that
would be specified from a trace control application. E.g. the user wants a type
of tracing that collects all information required to solve a category of
problem, and they get enabled automatically.

> > 
> > The specific module tracepoints should therefore only be enabled when both of
> > the following conditions are true:
> > 
> > A - the end-user want to trace
> > B - the module is loaded
> > 
> > But it looks like hooking on modinfo will only let you unconditionally enable
> > the module tracepoints during normal system operations (even when tracing is
> > off), or not at all unless you previously load the module (which does not fit
> > with the reality of distributions automagically loading modules on demand while
> > tracing runs).
> 
> Lets keep it simple please. Right now my proposal does more than what we
> currently have. Perhaps we could change the enabling to only enable if
> "tracing is on", or some /proc/sys/kernel/X flag is set.

Well, thinking a little more about it, I won't be using this way of enabling
tracepoints in my tracer, so please feel free to make it as simple as you like.
I'm just providing feedback on what the ftrace/perf end user experience will
look like and, sadly, it does not look good at all by the look of this proposal.

> 
> 
> > > 
> > > > Maybe I am missing something, but this interface seems to lack the layer of
> > > > finish we might want to put into a user-visible API. I don't really see how
> > > > distributions can hope to automate any of this for their end-user without making
> > > > a mess of the /etc/modprobe.d/ they ship with.
> > > 
> > > What distros enable tracepoints by default?
> > 
> > Do you mean enable as having a probe connected, or just CONFIG_TRACEPOINTS=y ?
> 
> I mean having the probe connected as distros already have
> CONFIG_TRACEPOINTS on. What was your meaning when you said "distro
> specifying a basic set of tracepoints to enable"?

I meant that distros can contain packages that are interested in a specific set
of tracepoints (views/analysis are tracepoint data consumers), so they can
specify a set of tracepoints to enable when tracing is activated.

> 
> 
> > 
> > > 
> > > If you want to enable a tracepoint on module load simply do:
> > > 
> > >  modprobe mymod trace_my_tracepoint=1
> > > 
> > > Otherwise modify your modprobe.d directory. This is the way users have
> > > been doing module parameters for years.
> > > 
> > > That's pretty simple to me.
> > 
> > Everything is always so much easier when your end-user target is yourself.
> > What are users for anyway ? :-P
> 
> Users are for testing code ;)
> 
> But that's a good question. As I wrote this because I'm purging my inbox
> and came across Yuanhan Liu's patch set. I'm curios to what Yuanhan's
> motivation for this change was.

Yep.

Hopefully my feedback can be of some use.

Thanks,

Mathieu
Steven Rostedt March 9, 2011, 2:01 a.m. UTC | #9
On Tue, 2011-03-08 at 20:17 -0500, Mathieu Desnoyers wrote:

> I'm afraid I cannot say, at this point, which distro I am refering to, because
> that would be a little forward of me to push news before official feature
> announcements.

Well, I guess I'm safe at saying it aint Red Hat ;)

> 
> And about the "default" tracepoints, let's mainly think about tracepoints that
> would be specified from a trace control application. E.g. the user wants a type
> of tracing that collects all information required to solve a category of
> problem, and they get enabled automatically.

Well, since tracepoints can change (come and go), this tool had better
be very flexible.


> Well, thinking a little more about it, I won't be using this way of enabling
> tracepoints in my tracer, so please feel free to make it as simple as you like.
> I'm just providing feedback on what the ftrace/perf end user experience will
> look like and, sadly, it does not look good at all by the look of this proposal.

Sadly it matters what the point of this change was for. 1) this does not
affect the way perf enables/disable tracepoints. I'm sure it could
easily add a syscall interface that would keep a nice wall from the
user. 2) it was to enable tracing in ftrace as soon as a module is
loaded. Ideally from a modprobe, not boot time tracing. Although, I
probably could add something to for that too. But that would come later.


> I meant that distros can contain packages that are interested in a specific set
> of tracepoints (views/analysis are tracepoint data consumers), so they can
> specify a set of tracepoints to enable when tracing is activated.

Yep, and this patch is not aimed at that. I am interesting in things
that analyze specific data. But this patch was not something to address
that. And it could easily exist with other means that do.

> > 
> > But that's a good question. As I wrote this because I'm purging my inbox
> > and came across Yuanhan Liu's patch set. I'm curios to what Yuanhan's
> > motivation for this change was.
> 
> Yep.

Yep, I'm looking forward to a response. For myself, I like this patch
because there has been times I needed to enable a tracepoint as soon as
the module was loaded and not a long time afterward (which is what
happens when you do modprobe and echo by hand).

> 
> Hopefully my feedback can be of some use.

For who?

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yuanhan Liu March 9, 2011, 2:01 a.m. UTC | #10
On Tue, Mar 08, 2011 at 07:52:04PM -0500, Steven Rostedt wrote:
> On Tue, 2011-03-08 at 19:29 -0500, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > On Tue, 2011-03-08 at 19:07 -0500, Mathieu Desnoyers wrote:

[snip]
 
> > 
> > > 
> > > If you want to enable a tracepoint on module load simply do:
> > > 
> > >  modprobe mymod trace_my_tracepoint=1
> > > 
> > > Otherwise modify your modprobe.d directory. This is the way users have
> > > been doing module parameters for years.
> > > 
> > > That's pretty simple to me.
> > 
> > Everything is always so much easier when your end-user target is yourself.
> > What are users for anyway ? :-P
> 
> Users are for testing code ;)

As another end-user, this looks fine (also simple) to me ;)

> 
> But that's a good question. As I wrote this because I'm purging my inbox
> and came across Yuanhan Liu's patch set. I'm curios to what Yuanhan's
> motivation for this change was.

The motivation of my original patch is simple: try to not miss any trace
events. I guess it's a kind of feature that trace(or module) should
have ;) 


Thanks,
Yuanhan Liu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Steven Rostedt March 9, 2011, 2:12 a.m. UTC | #11
On Wed, 2011-03-09 at 10:01 +0800, Yuanhan Liu wrote:

> > Users are for testing code ;)
> 
> As another end-user, this looks fine (also simple) to me ;)

Ah, you find this patch I have simple?

> 
> > 
> > But that's a good question. As I wrote this because I'm purging my inbox
> > and came across Yuanhan Liu's patch set. I'm curios to what Yuanhan's
> > motivation for this change was.
> 
> The motivation of my original patch is simple: try to not miss any trace
> events. I guess it's a kind of feature that trace(or module) should
> have ;) 

Hmm, I probably could add a trace option that simply tells ftrace to
enable events as modules are loaded.

Thanks!

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yuanhan Liu March 9, 2011, 2:19 a.m. UTC | #12
On Tue, Mar 08, 2011 at 09:12:14PM -0500, Steven Rostedt wrote:
> On Wed, 2011-03-09 at 10:01 +0800, Yuanhan Liu wrote:
> 
> > > Users are for testing code ;)
> > 
> > As another end-user, this looks fine (also simple) to me ;)
> 
> Ah, you find this patch I have simple?

Nop, I mean the way to enable it.

> 
> > 
> > > 
> > > But that's a good question. As I wrote this because I'm purging my inbox
> > > and came across Yuanhan Liu's patch set. I'm curios to what Yuanhan's
> > > motivation for this change was.
> > 
> > The motivation of my original patch is simple: try to not miss any trace
> > events. I guess it's a kind of feature that trace(or module) should
> > have ;) 
> 
> Hmm, I probably could add a trace option that simply tells ftrace to
> enable events as modules are loaded.
> 
> Thanks!
> 
> -- Steve
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers March 9, 2011, 2:30 a.m. UTC | #13
* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Tue, 2011-03-08 at 20:17 -0500, Mathieu Desnoyers wrote:
> 
> > 
> > And about the "default" tracepoints, let's mainly think about tracepoints that
> > would be specified from a trace control application. E.g. the user wants a type
> > of tracing that collects all information required to solve a category of
> > problem, and they get enabled automatically.
> 
> Well, since tracepoints can change (come and go), this tool had better
> be very flexible.

The idea is that a tool activates a set of tracepoints, gather trace data, and
then use the metainformation to figure out which of the tracepoints it requested
are there. If some of the tracepoints it needs are not there, it bails out.

> 
> 
> > Well, thinking a little more about it, I won't be using this way of enabling
> > tracepoints in my tracer, so please feel free to make it as simple as you like.
> > I'm just providing feedback on what the ftrace/perf end user experience will
> > look like and, sadly, it does not look good at all by the look of this proposal.
> 
> Sadly it matters what the point of this change was for. 1) this does not
> affect the way perf enables/disable tracepoints. I'm sure it could
> easily add a syscall interface that would keep a nice wall from the
> user. 2) it was to enable tracing in ftrace as soon as a module is
> loaded. Ideally from a modprobe, not boot time tracing. Although, I
> probably could add something to for that too. But that would come later.
> 
> 
> > I meant that distros can contain packages that are interested in a specific set
> > of tracepoints (views/analysis are tracepoint data consumers), so they can
> > specify a set of tracepoints to enable when tracing is activated.
> 
> Yep, and this patch is not aimed at that. I am interesting in things
> that analyze specific data. But this patch was not something to address
> that. And it could easily exist with other means that do.

As long as this method of enabling TRACE_EVENT probes located in modules before
these are loaded does not become the single and only one, I'm fine with it. It
makes sense for the kernel-developer use-case. Although past experience taught
me to be careful about the "we'll allow better interfaces to come later"
approach.

[...]

> > 
> > Hopefully my feedback can be of some use.
> 
> For who?

For who CC'd me on this thread. ;-)

Thanks,

Mathieu
Rusty Russell March 10, 2011, 11:33 p.m. UTC | #14
On Tue, 08 Mar 2011 17:18:04 -0500, Steven Rostedt <rostedt@goodmis.org> wrote:
> A few months ago it was suggested to have a way to enable tracepoints in
> a module when it is loaded. I tried various methods, but this one seems
> to be the least intrusive. In fact, it requires no modification to the
> module code.

This seems quite nice!

Importantly, do you like it better than the previous version?  I like it
because it doesn't touch my code, but that's not a fair test.

A few minor things:
1) Can we document this somewhere more persistent too? ftrace.txt?
2) Your documentation should probably just use "trace_foo_bar" and omit
   the "=1".  Because we can :)
3) Can we share more with param_set_bool?  Nice to have it in one place
   when someone decides the kernel really needs to accept "=true" or
   whatever.
4) =n should really unset the flag, so args fight correctly.

eg (wildly untested):

int ftrace_mod_param_set(const char *val, const struct kernel_param *kp)
{
	struct ftrace_event_call *call = kp->arg;
        struct kernel_param bool_kp;
        bool set;
        int err;

        /* We work exactly like param_set_bool. */
        bool_kp.arg = &set;
        bool_kp.flags = KPARAM_ISBOOL;
        err = param_set_bool(val, &bool_kp);
        if (err)
                return err;

       	/* Set flag to tell ftrace to enable this event on init */
        if (set)
                call->flags = TRACE_EVENT_FL_MOD_ENABLE;
        else
                call->flags &= ~TRACE_EVENT_FL_MOD_ENABLE;

	return 0;
}

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Steven Rostedt Aug. 13, 2013, 3:14 p.m. UTC | #15
On Fri, 11 Mar 2011 10:03:09 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

[ Resurrection from the dead! ]

> On Tue, 08 Mar 2011 17:18:04 -0500, Steven Rostedt <rostedt@goodmis.org> wrote:
> > A few months ago it was suggested to have a way to enable tracepoints in
> > a module when it is loaded. I tried various methods, but this one seems
> > to be the least intrusive. In fact, it requires no modification to the
> > module code.
> 
> This seems quite nice!
> 
> Importantly, do you like it better than the previous version?  I like it
> because it doesn't touch my code, but that's not a fair test.
> 
> A few minor things:
> 1) Can we document this somewhere more persistent too? ftrace.txt?

I'll need to update this.

> 2) Your documentation should probably just use "trace_foo_bar" and omit
>    the "=1".  Because we can :)

And it does.

> 3) Can we share more with param_set_bool?  Nice to have it in one place
>    when someone decides the kernel really needs to accept "=true" or
>    whatever.

I'll look into this.

> 4) =n should really unset the flag, so args fight correctly.
> 
> eg (wildly untested):
> 
> int ftrace_mod_param_set(const char *val, const struct kernel_param *kp)
> {
> 	struct ftrace_event_call *call = kp->arg;
>         struct kernel_param bool_kp;
>         bool set;
>         int err;
> 
>         /* We work exactly like param_set_bool. */
>         bool_kp.arg = &set;
>         bool_kp.flags = KPARAM_ISBOOL;
>         err = param_set_bool(val, &bool_kp);
>         if (err)
>                 return err;
> 
>        	/* Set flag to tell ftrace to enable this event on init */
>         if (set)
>                 call->flags = TRACE_EVENT_FL_MOD_ENABLE;
>         else
>                 call->flags &= ~TRACE_EVENT_FL_MOD_ENABLE;
> 
> 	return 0;
> }

I can add this too.

But the thing about this that bothers me is that there's no way to say,
"Enable all tracepoints in this module on load". I would like a way to
do that, but I don't know of a way to do that without modifying the
module code. Have any ideas? Basically, I would love to have:

insmod foo tracepoints=all

or something and have all tracepoints enabled.

I started playing with tricks in the include/trace/ftrace.h file to
automatically create a general module parameter, but modules like KVM
include multiple trace headers which means anything I make will be
duplicated in the module and wont work.

Well, I'll go back to this original patch set (as I've been asked three
times within the last few months if there's a way to have tracepoints
enabled on module load).

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathieu Desnoyers Aug. 13, 2013, 10:34 p.m. UTC | #16
* Steven Rostedt (rostedt@goodmis.org) wrote:
[...]
> But the thing about this that bothers me is that there's no way to say,
> "Enable all tracepoints in this module on load". I would like a way to
> do that, but I don't know of a way to do that without modifying the
> module code. Have any ideas? Basically, I would love to have:
> 
> insmod foo tracepoints=all
> 
> or something and have all tracepoints enabled.

If it can help, here is a very similar scenario: tracepoints in
lttng-ust (user land) deal with dynamically loaded libraries that are
loaded at any point during program execution, which is very much similar
to the kernel module use-case discussed here.

The way a user interacts with lttng to enable, e.g. all tracepoints
within "app_component" would be:

lttng enable-event -u 'app_component:*'

Then, whenever a library containing tracepoints is loaded, the
tracepoints contained within the library are registered to lttng-ust,
and it enables all tracepoint matching the user-specified wildcard.

What I like about this approach, if applied to kernel modules, is that
it does not require users to interact with module load parameters to
specify which tracepoints should be enabled: this is all done through
the regular tracer UI, thus greatly improving user experience.

Thoughts ?

Thanks,

Mathieu
Steven Rostedt Aug. 13, 2013, 11:09 p.m. UTC | #17
On Tue, 13 Aug 2013 18:34:53 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:


> What I like about this approach, if applied to kernel modules, is that
> it does not require users to interact with module load parameters to
> specify which tracepoints should be enabled: this is all done through
> the regular tracer UI, thus greatly improving user experience.
> 

I have thought about adding a file that would let you enable generic
tracepoints as they are created. Doesn't even need to be specifically
modules, but kprobes as well.

But that's agnostic to this patch. One thing I like about the patch is
that it has modinfo show you the available tracepoints in a module.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rusty Russell Aug. 15, 2013, 2:02 a.m. UTC | #18
Steven Rostedt <rostedt@goodmis.org> writes:
> But the thing about this that bothers me is that there's no way to say,
> "Enable all tracepoints in this module on load". I would like a way to
> do that, but I don't know of a way to do that without modifying the
> module code. Have any ideas? Basically, I would love to have:
>
> insmod foo tracepoints=all
>
> or something and have all tracepoints enabled.

"without modifying the module code"?  Why?  The code isn't that scary,
and this seems useful.

Cheers,
Rusty.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Steven Rostedt Aug. 15, 2013, 3:32 a.m. UTC | #19
On Thu, 15 Aug 2013 11:32:10 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> > But the thing about this that bothers me is that there's no way to say,
> > "Enable all tracepoints in this module on load". I would like a way to
> > do that, but I don't know of a way to do that without modifying the
> > module code. Have any ideas? Basically, I would love to have:
> >
> > insmod foo tracepoints=all
> >
> > or something and have all tracepoints enabled.
> 
> "without modifying the module code"?  Why?  The code isn't that scary,
> and this seems useful.

I'm not afraid of the code, I'm afraid of you ;-) I hear you have
killer puppies.


OK, then when I get some time, I may cook something up.

Thanks,

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

Patch
diff mbox series

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 47e3997..b672889 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -124,6 +124,8 @@  void tracing_record_cmdline(struct task_struct *tsk);
 
 struct event_filter;
 
+extern struct kernel_param_ops ftrace_mod_ops;
+
 enum trace_reg {
 	TRACE_REG_REGISTER,
 	TRACE_REG_UNREGISTER,
@@ -155,6 +157,7 @@  enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_RECORDED_CMD_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
+	TRACE_EVENT_FL_MOD_ENABLE_BIT,
 };
 
 enum {
@@ -162,6 +165,7 @@  enum {
 	TRACE_EVENT_FL_FILTERED		= (1 << TRACE_EVENT_FL_FILTERED_BIT),
 	TRACE_EVENT_FL_RECORDED_CMD	= (1 << TRACE_EVENT_FL_RECORDED_CMD_BIT),
 	TRACE_EVENT_FL_CAP_ANY		= (1 << TRACE_EVENT_FL_CAP_ANY_BIT),
+	TRACE_EVENT_FL_MOD_ENABLE	= (1 << TRACE_EVENT_FL_MOD_ENABLE_BIT),
 };
 
 struct ftrace_event_call {
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 3e68366..3d4a6ee 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -561,6 +561,22 @@  static inline void ftrace_test_probe_##call(void)			\
 #undef __get_dynamic_array
 #undef __get_str
 
+/*
+ * Add ftrace trace points in modules to be set by module
+ * parameters. This adds "trace_##call" as a module parameter.
+ * The user could enable trace points on module load with:
+ *  trace_##call=1 as a module parameter.
+ */
+#undef ftrace_mod_params
+#ifdef MODULE
+#define ftrace_mod_params(call)				\
+	module_param_cb(trace_##call, &ftrace_mod_ops,	\
+			&event_##call, 0644);		\
+	MODULE_INFO(tracepoint, #call)
+#else
+#define ftrace_mod_params(call)
+#endif
+
 #undef TP_printk
 #define TP_printk(fmt, args...) "\"" fmt "\", "  __stringify(args)
 
@@ -588,7 +604,8 @@  static struct ftrace_event_call __used event_##call = {			\
 	.print_fmt		= print_fmt_##template,			\
 };									\
 static struct ftrace_event_call __used					\
-__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call;\
+ftrace_mod_params(call)
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, call, proto, args, print)		\
@@ -602,7 +619,8 @@  static struct ftrace_event_call __used event_##call = {			\
 	.print_fmt		= print_fmt_##call,			\
 };									\
 static struct ftrace_event_call __used					\
-__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call
+__attribute__((section("_ftrace_events"))) *__event_##call = &event_##call; \
+ftrace_mod_params(call)
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
 
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 5f499e0..5223751 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1285,6 +1285,7 @@  static void trace_module_add_events(struct module *mod)
 {
 	struct ftrace_module_file_ops *file_ops = NULL;
 	struct ftrace_event_call **call, **start, **end;
+	int ret;
 
 	start = mod->trace_events;
 	end = mod->trace_events + mod->num_trace_events;
@@ -1300,6 +1301,14 @@  static void trace_module_add_events(struct module *mod)
 		__trace_add_event_call(*call, mod,
 				       &file_ops->id, &file_ops->enable,
 				       &file_ops->filter, &file_ops->format);
+		/* If the module tracepoint parameter was set */
+		if ((*call)->flags & TRACE_EVENT_FL_MOD_ENABLE) {
+			(*call)->flags = 0;
+			ret = ftrace_event_enable_disable(*call, 1);
+			if (ret < 0)
+				pr_warning("unable to enable tracepoint %s",
+					   (*call)->name);
+		}
 	}
 }
 
@@ -1457,6 +1466,49 @@  static __init int event_trace_init(void)
 }
 fs_initcall(event_trace_init);
 
+/* Allow modules to load with enabled trace points */
+int ftrace_mod_param_set(const char *val, const struct kernel_param *kp)
+{
+	struct ftrace_event_call *call = kp->arg;
+
+	/* This is set like param_set_bool() */
+
+	/* No equals means "set"... */
+	if (!val)
+		val = "1";
+
+	/* One of =[yYnN01] */
+	switch (val[0]) {
+	case 'y': case 'Y': case '1':
+		break;
+	case 'n': case 'N': case '0':
+		/* Do nothing */
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	/* Set flag to tell ftrace to enable this event on init */
+	call->flags = TRACE_EVENT_FL_MOD_ENABLE;
+
+	return 0;
+}
+
+int ftrace_mod_param_get(char *buffer, const struct kernel_param *kp)
+{
+	struct ftrace_event_call *call = kp->arg;
+
+	return sprintf(buffer, "%d",
+		   !!(call->flags &
+		     (TRACE_EVENT_FL_MOD_ENABLE | TRACE_EVENT_FL_ENABLED)));
+}
+
+struct kernel_param_ops ftrace_mod_ops = {
+	.set = ftrace_mod_param_set,
+	.get = ftrace_mod_param_get,
+};
+EXPORT_SYMBOL(ftrace_mod_ops);
+
 #ifdef CONFIG_FTRACE_STARTUP_TEST
 
 static DEFINE_SPINLOCK(test_spinlock);