by 平松雅巳 / HIRAMATU，MASAMI

[permalink] [raw]

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 <[email protected]>
> ---
> 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: [email protected]

2013-07-22 11:19:09

by 平松雅巳 / HIRAMATU，MASAMI

[permalink] [raw]

Subject: Re: [PATCH v3 2/9] tracing: add basic event trigger framework

(2013/07/20 0:09), Tom Zanussi wrote:
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index af6eb2c..d3e8626 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1021,6 +1021,43 @@ extern int event_trace_del_tracer(struct trace_array *tr);
> extern struct mutex event_mutex;
> extern struct list_head ftrace_events;
>
> +extern const struct file_operations event_trigger_fops;
> +
> +extern int register_trigger_cmds(void);
> +
> +struct event_trigger_ops {
> + void (*func)(void **data);
> + int (*init)(struct event_trigger_ops *ops,
> + void **data);
> + void (*free)(struct event_trigger_ops *ops,
> + void **data);
> + int (*print)(struct seq_file *m,
> + struct event_trigger_ops *ops,
> + void *data);
> +};

Please add comments what each ops does. :)

> +
> +struct event_command {
> + struct list_head list;
> + char *name;
> + enum trigger_mode trigger_mode;
> + int (*func)(struct event_command *cmd_ops,
> + void *cmd_data, char *glob, char *cmd,
> + char *params, int enable);
> + int (*reg)(char *glob,
> + struct event_trigger_ops *trigger_ops,
> + void *trigger_data, void *cmd_data);
> + void (*unreg)(char *glob,
> + struct event_trigger_ops *trigger_ops,
> + void *trigger_data, void *cmd_data);
> + int (*set_filter)(char *filter_str,
> + void *trigger_data,
> + void *cmd_data);
> + struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param);
> +};

Ditto.

[...]
> +static int event_trigger_regex_open(struct inode *inode, struct file *file)
> +{
> + struct trigger_iterator *iter;
> + int ret = 0;
> +
> + iter = kzalloc(sizeof(*iter), GFP_KERNEL);
> + if (!iter)
> + return -ENOMEM;
> +
> + iter->file = inode->i_private;
> +
> + mutex_lock(&event_mutex);
> +
> + if (file->f_mode & FMODE_READ) {
> + ret = seq_open(file, &event_triggers_seq_ops);
> + if (!ret) {
> + struct seq_file *m = file->private_data;
> + m->private = iter;
> + } else {
> + /* Failed */
> + kfree(iter);
> + }
> + } else
> + file->private_data = iter;
> +

Perhaps, unfortunately, this will not work correctly if currently
discussing bugfix approach is applied.

Oleg, would you help us what this should be?

> + mutex_unlock(&event_mutex);
> +
> + return ret;
> +}

[...]

> +static int unregister_event_command(struct event_command *cmd,
> + struct list_head *cmd_list,
> + struct mutex *cmd_list_mutex)
> +{
> + struct event_command *p, *n;
> + int ret = -ENODEV;
> +
> + mutex_lock(cmd_list_mutex);
> + list_for_each_entry_safe(p, n, cmd_list, list) {
> + if (strcmp(cmd->name, p->name) == 0) {
> + ret = 0;
> + list_del_init(&p->list);
> + goto out_unlock;
> + }
> + }
> + out_unlock:
> + mutex_unlock(cmd_list_mutex);
> +
> + return ret;
> +}

OK, it seems that this is used only for rollback process in __init
functions. If so, it should have __init (and a comment),
or we need a refcount to check no one uses the command anymore.

Thank you,

--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]