Message-ID: <51ED12CC.4090609@hitachi.com>
Date:	Mon, 22 Jul 2013 20:09:00 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Tom Zanussi <tom.zanussi@...ux.intel.com>
Cc:	rostedt@...dmis.org, jovi.zhangwei@...wei.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/9] tracing: add 'traceon' and 'traceoff' event trigger
 commands

(2013/07/20 0:09), Tom Zanussi wrote:
> Add 'traceon' and 'traceoff' ftrace_func_command commands.  traceon
> and traceoff event triggers are added by the user via these commands
> in a similar way and using practically the same syntax as the
> analagous 'traceon' and 'traceoff' ftrace function commands, but
> instead of writing to the set_ftrace_filter file, the traceon and
> traceoff triggers are written to the per-event 'trigger' files:
> 
>     echo 'traceon' > .../tracing/events/somesys/someevent/trigger
>     echo 'traceoff' > .../tracing/events/somesys/someevent/trigger
> 
> The above command will turn tracing on or off whenever someevent is
> hit.
> 
> This also adds a 'count' version that limits the number of times the
> command will be invoked:
> 
>     echo 'traceon:N' > .../tracing/events/somesys/someevent/trigger
>     echo 'traceoff:N' > .../tracing/events/somesys/someevent/trigger
> 
> Where N is the number of times the command will be invoked.
> 
> The above commands will will turn tracing on or off whenever someevent
> is hit, but only N times.
> 
> The event_trigger_init() and event_trigger_free() are meant to be
> common implementations of the event_trigger_ops init() and free() ops.
> Most trigger_ops implementations will use these, but some will
> override and possibly reuse them.

Please add a comment in the code. (perhaps, a separator comment is enough)

event_trigger_callback(), register_trigger and unregister_trigger in 2/9
are common for event_command func(), reg() and unreg() too.
I think it is better to move those common code in this or previous patch.

Thank you,

> Signed-off-by: Tom Zanussi <tom.zanussi@...ux.intel.com>
> ---
>  include/linux/ftrace_event.h        |   1 +
>  kernel/trace/trace_events_trigger.c | 313 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 314 insertions(+)
> 
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 6cd5bbc..c794686 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -316,6 +316,7 @@ struct ftrace_event_file {
>  
>  enum trigger_mode {
>  	TM_NONE			= (0),
> +	TM_TRACE_ONOFF		= (1 << 0),
>  };
>  
>  extern void destroy_preds(struct ftrace_event_call *call);
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 1f0565c..f2b97b6 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -355,7 +355,320 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
>  		data->ops->free(data->ops, (void **)&data);
>  }
>  
> +int
> +event_trigger_print(const char *name, struct seq_file *m,
> +		    void *data, char *filter_str)
> +{
> +	long count = (long)data;
> +
> +	seq_printf(m, "%s", name);
> +
> +	if (count == -1)
> +		seq_puts(m, ":unlimited");
> +	else
> +		seq_printf(m, ":count=%ld", count);
> +
> +	if (filter_str)
> +		seq_printf(m, " if %s\n", filter_str);
> +	else
> +		seq_puts(m, "\n");
> +
> +	return 0;
> +}
> +
> +static int
> +event_trigger_init(struct event_trigger_ops *ops, void **_data)
> +{
> +	struct event_trigger_data **p = (struct event_trigger_data **)_data;
> +	struct event_trigger_data *data = *p;
> +
> +	data->ref++;
> +	return 0;
> +}
> +
> +static void
> +event_trigger_free(struct event_trigger_ops *ops, void **_data)
> +{
> +	struct event_trigger_data **p = (struct event_trigger_data **)_data;
> +	struct event_trigger_data *data = *p;
> +
> +	if (WARN_ON_ONCE(data->ref <= 0))
> +		return;
> +
> +	data->ref--;
> +	if (!data->ref)
> +		kfree(data);
> +}
> +
> +static int
> +event_trigger_callback(struct event_command *cmd_ops, void *cmd_data,
> +		       char *glob, char *cmd, char *param, int enabled)
> +{
> +	struct event_trigger_ops *trigger_ops;
> +	struct event_trigger_data *trigger_data;
> +	char *trigger = NULL;
> +	char *number;
> +	int ret;
> +
> +	if (!enabled)
> +		return -EINVAL;
> +
> +	/* separate the trigger from the filter (t:n [if filter]) */
> +	if (param && isdigit(param[0]))
> +		trigger = strsep(&param, " \t");
> +
> +	mutex_lock(&event_mutex);
> +
> +	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
> +
> +	ret = -ENOMEM;
> +	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> +	if (!trigger_data)
> +		goto out;
> +
> +	trigger_data->count = -1;
> +	trigger_data->ops = trigger_ops;
> +	trigger_data->mode = cmd_ops->trigger_mode;
> +	INIT_LIST_HEAD(&trigger_data->list);
> +
> +	if (glob[0] == '!') {
> +		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, cmd_data);
> +		kfree(trigger_data);
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	if (trigger) {
> +		number = strsep(&trigger, ":");
> +
> +		ret = -EINVAL;
> +		if (!strlen(number))
> +			goto out_free;
> +
> +		/*
> +		 * We use the callback data field (which is a pointer)
> +		 * as our counter.
> +		 */
> +		ret = kstrtoul(number, 0, &trigger_data->count);
> +		if (ret)
> +			goto out_free;
> +	}
> +
> +	if (!param) /* if param is non-empty, it's supposed to be a filter */
> +		goto out_reg;
> +
> +	if (!cmd_ops->set_filter)
> +		goto out_reg;
> +
> +	ret = cmd_ops->set_filter(param, trigger_data, cmd_data);
> +	if (ret < 0)
> +		goto out_free;
> +
> + out_reg:
> +	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, cmd_data);
> +	/*
> +	 * The above returns on success the # of functions enabled,
> +	 * but if it didn't find any functions it returns zero.
> +	 * Consider no functions a failure too.
> +	 */
> +	if (!ret) {
> +		ret = -ENOENT;
> +		goto out_free;
> +	} else if (ret < 0)
> +		goto out_free;
> +	ret = 0;
> + out:
> +	mutex_unlock(&event_mutex);
> +	return ret;
> +
> + out_free:
> +	kfree(trigger_data);
> +	goto out;
> +}
> +
> +static void
> +traceon_trigger(void **_data)
> +{
> +	struct event_trigger_data **p = (struct event_trigger_data **)_data;
> +	struct event_trigger_data *data = *p;
> +
> +	if (!data)
> +		return;
> +
> +	if (tracing_is_on())
> +		return;
> +
> +	tracing_on();
> +}
> +
> +static void
> +traceon_count_trigger(void **_data)
> +{
> +	struct event_trigger_data **p = (struct event_trigger_data **)_data;
> +	struct event_trigger_data *data = *p;
> +
> +	if (!data)
> +		return;
> +
> +	if (!data->count)
> +		return;
> +
> +	if (data->count != -1)
> +		(data->count)--;
> +
> +	traceon_trigger(_data);
> +}
> +
> +static void
> +traceoff_trigger(void **_data)
> +{
> +	struct event_trigger_data **p = (struct event_trigger_data **)_data;
> +	struct event_trigger_data *data = *p;
> +
> +	if (!data)
> +		return;
> +
> +	if (!tracing_is_on())
> +		return;
> +
> +	tracing_off();
> +}
> +
> +static void
> +traceoff_count_trigger(void **_data)
> +{
> +	struct event_trigger_data **p = (struct event_trigger_data **)_data;
> +	struct event_trigger_data *data = *p;
> +
> +	if (!data)
> +		return;
> +
> +	if (!data->count)
> +		return;
> +
> +	if (data->count != -1)
> +		(data->count)--;
> +
> +	traceoff_trigger(_data);
> +}
> +
> +static int
> +traceon_trigger_print(struct seq_file *m, struct event_trigger_ops *ops,
> +		      void *_data)
> +{
> +	struct event_trigger_data *data = _data;
> +
> +	return event_trigger_print("traceon", m, (void *)data->count,
> +				   data->filter_str);
> +}
> +
> +static int
> +traceoff_trigger_print(struct seq_file *m, struct event_trigger_ops *ops,
> +		       void *_data)
> +{
> +	struct event_trigger_data *data = _data;
> +
> +	return event_trigger_print("traceoff", m, (void *)data->count,
> +				   data->filter_str);
> +}
> +
> +static struct event_trigger_ops traceon_trigger_ops = {
> +	.func			= traceon_trigger,
> +	.print			= traceon_trigger_print,
> +	.init			= event_trigger_init,
> +	.free			= event_trigger_free,
> +};
> +
> +static struct event_trigger_ops traceon_count_trigger_ops = {
> +	.func			= traceon_count_trigger,
> +	.print			= traceon_trigger_print,
> +	.init			= event_trigger_init,
> +	.free			= event_trigger_free,
> +};
> +
> +static struct event_trigger_ops traceoff_trigger_ops = {
> +	.func			= traceoff_trigger,
> +	.print			= traceoff_trigger_print,
> +	.init			= event_trigger_init,
> +	.free			= event_trigger_free,
> +};
> +
> +static struct event_trigger_ops traceoff_count_trigger_ops = {
> +	.func			= traceoff_count_trigger,
> +	.print			= traceoff_trigger_print,
> +	.init			= event_trigger_init,
> +	.free			= event_trigger_free,
> +};
> +
> +static struct event_trigger_ops *
> +onoff_get_trigger_ops(char *cmd, char *param)
> +{
> +	struct event_trigger_ops *ops;
> +
> +	/* we register both traceon and traceoff to this callback */
> +	if (strcmp(cmd, "traceon") == 0)
> +		ops = param ? &traceon_count_trigger_ops :
> +			&traceon_trigger_ops;
> +	else
> +		ops = param ? &traceoff_count_trigger_ops :
> +			&traceoff_trigger_ops;
> +
> +	return ops;
> +}
> +
> +static struct event_command trigger_traceon_cmd = {
> +	.name			= "traceon",
> +	.trigger_mode		= TM_TRACE_ONOFF,
> +	.func			= event_trigger_callback,
> +	.reg			= register_trigger,
> +	.unreg			= unregister_trigger,
> +	.get_trigger_ops	= onoff_get_trigger_ops,
> +};
> +
> +static struct event_command trigger_traceoff_cmd = {
> +	.name			= "traceoff",
> +	.trigger_mode		= TM_TRACE_ONOFF,
> +	.func			= event_trigger_callback,
> +	.reg			= register_trigger,
> +	.unreg			= unregister_trigger,
> +	.get_trigger_ops	= onoff_get_trigger_ops,
> +};
> +
> +static __init void unregister_trigger_traceon_traceoff_cmds(void)
> +{
> +	unregister_event_command(&trigger_traceon_cmd,
> +				 &trigger_commands,
> +				 &trigger_cmd_mutex);
> +	unregister_event_command(&trigger_traceoff_cmd,
> +				 &trigger_commands,
> +				 &trigger_cmd_mutex);
> +}
> +
> +static __init int register_trigger_traceon_traceoff_cmds(void)
> +{
> +	int ret;
> +
> +	ret = register_event_command(&trigger_traceon_cmd, &trigger_commands,
> +				     &trigger_cmd_mutex);
> +	if (WARN_ON(ret < 0))
> +		return ret;
> +	ret = register_event_command(&trigger_traceoff_cmd, &trigger_commands,
> +				     &trigger_cmd_mutex);
> +	if (WARN_ON(ret < 0))
> +		unregister_trigger_traceon_traceoff_cmds();
> +
> +	return ret;
> +}
> +
>  __init int register_trigger_cmds(void)
>  {
> +	int ret;
> +
> +	ret = register_trigger_traceon_traceoff_cmds();
> +	if (ret) {
> +		unregister_trigger_traceon_traceoff_cmds();
> +		return ret;
> +	}
> +
>  	return 0;
>  }
> 


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


--
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/

Powered by blists - more mailing lists