Re: [PATCH v9 04/10] tracing: Add 'snapshot' event trigger command

[Tom Zanussi <tom.zanussi@linux.intel.com>]

On Fri, 2013-09-13 at 16:01 -0400, Steven Rostedt wrote:
> On Sat,  7 Sep 2013 10:29:00 -0500
> Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
> 
> 
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -696,6 +696,74 @@ static struct event_command trigger_traceoff_cmd = {
> >  	.get_trigger_ops	= onoff_get_trigger_ops,
> >  };
> >  
> > +static void
> > +snapshot_trigger(struct event_trigger_data *data)
> > +{
> > +	tracing_snapshot();
> > +}
> 
> If CONFIG_TRACER_SNAPSHOT is not defined, then we should not bother
> implementing the snapshot trigger. This should be encapsulated around
> ifdefs.

OK, I guess I was just trying to avoid the ifdef since
tracing_snapshot() is already ifdef'ed out (but with a WARN_ONCE()) if
CONFIG_TRACER_SNAPSHOT isn't defined.

I agree though, it would be better to just ignore all the snapshot
trigger code if that's the case.  Same for the stacktrace trigger,
though as much as I hate to put big ifdefs in the main code...

> 
> > +
> > +static void
> > +snapshot_count_trigger(struct event_trigger_data *data)
> > +{
> > +	if (!data->count)
> > +		return;
> > +
> > +	if (data->count != -1)
> > +		(data->count)--;
> > +
> > +	snapshot_trigger(data);
> > +}
> > +
> > +static int
> > +register_snapshot_trigger(char *glob, struct event_trigger_ops *ops,
> > +			  struct event_trigger_data *data,
> > +			  struct ftrace_event_file *file)
> > +{
> > +	int ret = register_trigger(glob, ops, data, file);
> > +
> > +	if (ret > 0)
> > +		ftrace_alloc_snapshot();
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +snapshot_trigger_print(struct seq_file *m, struct event_trigger_ops *ops,
> > +		       struct event_trigger_data *data)
> > +{
> > +	return event_trigger_print("snapshot", m, (void *)data->count,
> > +				   data->filter_str);
> > +}
> > +
> > +static struct event_trigger_ops snapshot_trigger_ops = {
> > +	.func			= snapshot_trigger,
> > +	.print			= snapshot_trigger_print,
> > +	.init			= event_trigger_init,
> > +	.free			= event_trigger_free,
> > +};
> > +
> > +static struct event_trigger_ops snapshot_count_trigger_ops = {
> > +	.func			= snapshot_count_trigger,
> > +	.print			= snapshot_trigger_print,
> > +	.init			= event_trigger_init,
> > +	.free			= event_trigger_free,
> > +};
> > +
> > +static struct event_trigger_ops *
> > +snapshot_get_trigger_ops(char *cmd, char *param)
> > +{
> > +	return param ? &snapshot_count_trigger_ops : &snapshot_trigger_ops;
> > +}
> > +
> > +static struct event_command trigger_snapshot_cmd = {
> > +	.name			= "snapshot",
> > +	.trigger_type		= ETT_SNAPSHOT,
> > +	.func			= event_trigger_callback,
> > +	.reg			= register_snapshot_trigger,
> > +	.unreg			= unregister_trigger,
> > +	.get_trigger_ops	= snapshot_get_trigger_ops,
> > +};
> > +
> >  static __init void unregister_trigger_traceon_traceoff_cmds(void)
> >  {
> >  	unregister_event_command(&trigger_traceon_cmd);
> > @@ -726,5 +794,11 @@ __init int register_trigger_cmds(void)
> >  		return ret;
> >  	}
> >  
> > +	ret = register_event_command(&trigger_snapshot_cmd);
> > +	if (WARN_ON(ret < 0)) {
> > +		unregister_trigger_traceon_traceoff_cmds();
> 
> If the snapshot trigger fails, why remove the traceon_traceoff trigger
> if it succeeded? Is there some dependency that we should be worried
> about?
> 
> Or is this just saying "if once trigger fails, they all fail!"?
> 

Right, that's all its saying, there's no dependency.  I guess it would
be fine to just continue with whatever triggers did/will register
successfully - the WARN_ON() will show the failure for a given trigger,
no need to disable everything.

Tom

> -- Steve
> 
> > +		return ret;
> > +	}
> > +
> >  	return 0;
> >  }
>