Hi Masami, On Mon, 2015-07-20 at 23:57 +0900, Masami Hiramatsu wrote: > On 2015/07/17 2:22, Tom Zanussi wrote: > > Similar to enable_event/disable_event triggers, these triggers enable > > and disable the aggregation of events into maps rather than enabling > > and disabling their writing into the trace buffer. > > > > They can be used to automatically start and stop hist triggers based > > on a matching filter condition. > > > > If there's a paused hist trigger on system:event, the following would > > start it when the filter condition was hit: > > > > # echo enable_hist:system:event [ if filter] > event/trigger > > > > And the following would disable a running system:event hist trigger: > > > > # echo disable_hist:system:event [ if filter] > event/trigger > > > > See Documentation/trace/events.txt for real examples. > > Hmm, do we really need this? Since we've already had multiple instances, > if someone wants to make histogram separated from event logger, he/she can > make another instance for that, and disable/enable event itself. > > I'm considering if we accept this methods, we'll need to accept another > enable/disable triggers for each action too in the future. >
OK, I haven't implemented multiple instances yet, but if I understand you correctly, what you're suggesting is that we can accomplish the same thing, by setting up a disabled histogram on system:event and then simply using the existing enable_event:system:event trigger to turn it on. Likewise the opposite to disable it. I guess we need to add the histogram instance name to the syntax of enable_event:system:event to be able to make the distinction between a histogram and the current behavior of enabling logging. So here's what we currently have. This sets up a histogram and starts it running, and the user cats event/hist to get the results: # echo hist:keys=xxx > event1/trigger # cat event1/hist And separately the existing enable_event trigger, which enables event1 (starts it logging to the event logger, and has nothing to do with histograms) when event2 is hit: # echo enable_event:system:event1 > event2/trigger So to extend enable_event to support histograms, we need to be able to do this, first set up a paused histogram: # echo hist:keys=xxx:pause > event1/trigger # cat event1/hist Which would be enabled via enable_event like this: # echo enable_event:system:event1:hist > event2/trigger Of course 'hist' refers to the initial single-instance histogram - if we had multiple instances, 'hist' would be replaced by the instance name e.g. to set up two different histograms, each with a different filter: # echo hist:keys=xxx:pause if filter1 > event1/trigger # cat event1/hist # echo hist:keys=xxx:pause if filter2 > event1/trigger # cat event1/hist2 To enable the first histogram when event2 is hit: # echo enable_event:system:event1:hist > event2/trigger And to enable the second histogram when event2 is hit: # echo enable_event:system:event1:hist2 > event2/trigger Does that align with what you were thinking regarding both instances and the enable/disable_event triggers? If not, some more explanation and examples would help ;-) Thanks, Tom > Thank you, > > > > > Signed-off-by: Tom Zanussi <[email protected]> > > --- > > include/linux/trace_events.h | 1 + > > kernel/trace/trace.c | 11 ++++ > > kernel/trace/trace.h | 32 ++++++++++ > > kernel/trace/trace_events_hist.c | 115 > > ++++++++++++++++++++++++++++++++++++ > > kernel/trace/trace_events_trigger.c | 71 ++++++++++++---------- > > 5 files changed, 199 insertions(+), 31 deletions(-) > > > > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > > index 0faf48b..0f3ffdd 100644 > > --- a/include/linux/trace_events.h > > +++ b/include/linux/trace_events.h > > @@ -411,6 +411,7 @@ enum event_trigger_type { > > ETT_STACKTRACE = (1 << 2), > > ETT_EVENT_ENABLE = (1 << 3), > > ETT_EVENT_HIST = (1 << 4), > > + ETT_HIST_ENABLE = (1 << 5), > > }; > > > > extern int filter_match_preds(struct event_filter *filter, void *rec); > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index 16c64a2..c581750 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -3761,6 +3761,10 @@ static const char readme_msg[] = > > "\t trigger: traceon, traceoff\n" > > "\t enable_event:<system>:<event>\n" > > "\t disable_event:<system>:<event>\n" > > +#ifdef CONFIG_HIST_TRIGGERS > > + "\t enable_hist:<system>:<event>\n" > > + "\t disable_hist:<system>:<event>\n" > > +#endif > > #ifdef CONFIG_STACKTRACE > > "\t\t stacktrace\n" > > #endif > > @@ -3836,6 +3840,13 @@ static const char readme_msg[] = > > "\t restart a paused hist trigger.\n\n" > > "\t The 'clear' param will clear the contents of a running hist\n" > > "\t trigger and leave its current paused/active state.\n\n" > > + "\t The enable_hist and disable_hist triggers can be used to\n" > > + "\t have one event conditionally start and stop another event's\n" > > + "\t already-attached hist trigger. Any number of enable_hist\n" > > + "\t and disable_hist triggers can be attached to a given event,\n" > > + "\t allowing that event to kick off and stop aggregations on\n" > > + "\t a host of other events. See Documentation/trace/events.txt\n" > > + "\t for examples.\n" > > #endif > > ; > > > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > > index e6cb781..5e2e3b0 100644 > > --- a/kernel/trace/trace.h > > +++ b/kernel/trace/trace.h > > @@ -1102,8 +1102,10 @@ extern const struct file_operations event_hist_fops; > > > > #ifdef CONFIG_HIST_TRIGGERS > > extern int register_trigger_hist_cmd(void); > > +extern int register_trigger_hist_enable_disable_cmds(void); > > #else > > static inline int register_trigger_hist_cmd(void) { return 0; } > > +static inline int register_trigger_hist_enable_disable_cmds(void) { return > > 0; } > > #endif > > > > extern int register_trigger_cmds(void); > > @@ -1121,6 +1123,34 @@ struct event_trigger_data { > > struct list_head list; > > }; > > > > +/* Avoid typos */ > > +#define ENABLE_EVENT_STR "enable_event" > > +#define DISABLE_EVENT_STR "disable_event" > > +#define ENABLE_HIST_STR "enable_hist" > > +#define DISABLE_HIST_STR "disable_hist" > > + > > +struct enable_trigger_data { > > + struct trace_event_file *file; > > + bool enable; > > + bool hist; > > +}; > > + > > +extern int event_enable_trigger_print(struct seq_file *m, > > + struct event_trigger_ops *ops, > > + struct event_trigger_data *data); > > +extern void event_enable_trigger_free(struct event_trigger_ops *ops, > > + struct event_trigger_data *data); > > +extern int event_enable_trigger_func(struct event_command *cmd_ops, > > + struct trace_event_file *file, > > + char *glob, char *cmd, char *param); > > +extern int event_enable_register_trigger(char *glob, > > + struct event_trigger_ops *ops, > > + struct event_trigger_data *data, > > + struct trace_event_file *file); > > +extern void event_enable_unregister_trigger(char *glob, > > + struct event_trigger_ops *ops, > > + struct event_trigger_data *test, > > + struct trace_event_file *file); > > extern void trigger_data_free(struct event_trigger_data *data); > > extern int event_trigger_init(struct event_trigger_ops *ops, > > struct event_trigger_data *data); > > @@ -1134,6 +1164,8 @@ extern int set_trigger_filter(char *filter_str, > > struct event_trigger_data *trigger_data, > > struct trace_event_file *file); > > extern int register_event_command(struct event_command *cmd); > > +extern int unregister_event_command(struct event_command *cmd); > > +extern int register_trigger_hist_enable_disable_cmds(void); > > > > /** > > * struct event_trigger_ops - callbacks for trace event triggers > > diff --git a/kernel/trace/trace_events_hist.c > > b/kernel/trace/trace_events_hist.c > > index 4ba7645..6a43611 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -1345,3 +1345,118 @@ __init int register_trigger_hist_cmd(void) > > > > return ret; > > } > > + > > +static void > > +hist_enable_trigger(struct event_trigger_data *data, void *rec) > > +{ > > + struct enable_trigger_data *enable_data = data->private_data; > > + struct event_trigger_data *test; > > + > > + list_for_each_entry_rcu(test, &enable_data->file->triggers, list) { > > + if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) { > > + if (enable_data->enable) > > + test->paused = false; > > + else > > + test->paused = true; > > + break; > > + } > > + } > > +} > > + > > +static void > > +hist_enable_count_trigger(struct event_trigger_data *data, void *rec) > > +{ > > + if (!data->count) > > + return; > > + > > + if (data->count != -1) > > + (data->count)--; > > + > > + hist_enable_trigger(data, rec); > > +} > > + > > +static struct event_trigger_ops hist_enable_trigger_ops = { > > + .func = hist_enable_trigger, > > + .print = event_enable_trigger_print, > > + .init = event_trigger_init, > > + .free = event_enable_trigger_free, > > +}; > > + > > +static struct event_trigger_ops hist_enable_count_trigger_ops = { > > + .func = hist_enable_count_trigger, > > + .print = event_enable_trigger_print, > > + .init = event_trigger_init, > > + .free = event_enable_trigger_free, > > +}; > > + > > +static struct event_trigger_ops hist_disable_trigger_ops = { > > + .func = hist_enable_trigger, > > + .print = event_enable_trigger_print, > > + .init = event_trigger_init, > > + .free = event_enable_trigger_free, > > +}; > > + > > +static struct event_trigger_ops hist_disable_count_trigger_ops = { > > + .func = hist_enable_count_trigger, > > + .print = event_enable_trigger_print, > > + .init = event_trigger_init, > > + .free = event_enable_trigger_free, > > +}; > > + > > +static struct event_trigger_ops * > > +hist_enable_get_trigger_ops(char *cmd, char *param) > > +{ > > + struct event_trigger_ops *ops; > > + bool enable; > > + > > + enable = (strcmp(cmd, ENABLE_HIST_STR) == 0); > > + > > + if (enable) > > + ops = param ? &hist_enable_count_trigger_ops : > > + &hist_enable_trigger_ops; > > + else > > + ops = param ? &hist_disable_count_trigger_ops : > > + &hist_disable_trigger_ops; > > + > > + return ops; > > +} > > + > > +static struct event_command trigger_hist_enable_cmd = { > > + .name = ENABLE_HIST_STR, > > + .trigger_type = ETT_HIST_ENABLE, > > + .func = event_enable_trigger_func, > > + .reg = event_enable_register_trigger, > > + .unreg = event_enable_unregister_trigger, > > + .get_trigger_ops = hist_enable_get_trigger_ops, > > + .set_filter = set_trigger_filter, > > +}; > > + > > +static struct event_command trigger_hist_disable_cmd = { > > + .name = DISABLE_HIST_STR, > > + .trigger_type = ETT_HIST_ENABLE, > > + .func = event_enable_trigger_func, > > + .reg = event_enable_register_trigger, > > + .unreg = event_enable_unregister_trigger, > > + .get_trigger_ops = hist_enable_get_trigger_ops, > > + .set_filter = set_trigger_filter, > > +}; > > + > > +static __init void unregister_trigger_hist_enable_disable_cmds(void) > > +{ > > + unregister_event_command(&trigger_hist_enable_cmd); > > + unregister_event_command(&trigger_hist_disable_cmd); > > +} > > + > > +__init int register_trigger_hist_enable_disable_cmds(void) > > +{ > > + int ret; > > + > > + ret = register_event_command(&trigger_hist_enable_cmd); > > + if (WARN_ON(ret < 0)) > > + return ret; > > + ret = register_event_command(&trigger_hist_disable_cmd); > > + if (WARN_ON(ret < 0)) > > + unregister_trigger_hist_enable_disable_cmds(); > > + > > + return ret; > > +} > > diff --git a/kernel/trace/trace_events_trigger.c > > b/kernel/trace/trace_events_trigger.c > > index e80f30b..9490d8f 100644 > > --- a/kernel/trace/trace_events_trigger.c > > +++ b/kernel/trace/trace_events_trigger.c > > @@ -338,7 +338,7 @@ __init int register_event_command(struct event_command > > *cmd) > > * Currently we only unregister event commands from __init, so mark > > * this __init too. > > */ > > -static __init int unregister_event_command(struct event_command *cmd) > > +__init int unregister_event_command(struct event_command *cmd) > > { > > struct event_command *p, *n; > > int ret = -ENODEV; > > @@ -1052,15 +1052,6 @@ static __init void > > unregister_trigger_traceon_traceoff_cmds(void) > > unregister_event_command(&trigger_traceoff_cmd); > > } > > > > -/* Avoid typos */ > > -#define ENABLE_EVENT_STR "enable_event" > > -#define DISABLE_EVENT_STR "disable_event" > > - > > -struct enable_trigger_data { > > - struct trace_event_file *file; > > - bool enable; > > -}; > > - > > static void > > event_enable_trigger(struct event_trigger_data *data, void *rec) > > { > > @@ -1090,14 +1081,16 @@ event_enable_count_trigger(struct > > event_trigger_data *data, void *rec) > > event_enable_trigger(data, rec); > > } > > > > -static int > > -event_enable_trigger_print(struct seq_file *m, struct event_trigger_ops > > *ops, > > - struct event_trigger_data *data) > > +int event_enable_trigger_print(struct seq_file *m, > > + struct event_trigger_ops *ops, > > + struct event_trigger_data *data) > > { > > struct enable_trigger_data *enable_data = data->private_data; > > > > seq_printf(m, "%s:%s:%s", > > - enable_data->enable ? ENABLE_EVENT_STR : DISABLE_EVENT_STR, > > + enable_data->hist ? > > + (enable_data->enable ? ENABLE_HIST_STR : DISABLE_HIST_STR) : > > + (enable_data->enable ? ENABLE_EVENT_STR : DISABLE_EVENT_STR), > > enable_data->file->event_call->class->system, > > trace_event_name(enable_data->file->event_call)); > > > > @@ -1114,9 +1107,8 @@ event_enable_trigger_print(struct seq_file *m, struct > > event_trigger_ops *ops, > > return 0; > > } > > > > -static void > > -event_enable_trigger_free(struct event_trigger_ops *ops, > > - struct event_trigger_data *data) > > +void event_enable_trigger_free(struct event_trigger_ops *ops, > > + struct event_trigger_data *data) > > { > > struct enable_trigger_data *enable_data = data->private_data; > > > > @@ -1161,10 +1153,9 @@ static struct event_trigger_ops > > event_disable_count_trigger_ops = { > > .free = event_enable_trigger_free, > > }; > > > > -static int > > -event_enable_trigger_func(struct event_command *cmd_ops, > > - struct trace_event_file *file, > > - char *glob, char *cmd, char *param) > > +int event_enable_trigger_func(struct event_command *cmd_ops, > > + struct trace_event_file *file, > > + char *glob, char *cmd, char *param) > > { > > struct trace_event_file *event_enable_file; > > struct enable_trigger_data *enable_data; > > @@ -1173,6 +1164,7 @@ event_enable_trigger_func(struct event_command > > *cmd_ops, > > struct trace_array *tr = file->tr; > > const char *system; > > const char *event; > > + bool hist = false; > > char *trigger; > > char *number; > > bool enable; > > @@ -1197,8 +1189,15 @@ event_enable_trigger_func(struct event_command > > *cmd_ops, > > if (!event_enable_file) > > goto out; > > > > - enable = strcmp(cmd, ENABLE_EVENT_STR) == 0; > > +#ifdef CONFIG_HIST_TRIGGERS > > + hist = ((strcmp(cmd, ENABLE_HIST_STR) == 0) || > > + (strcmp(cmd, DISABLE_HIST_STR) == 0)); > > > > + enable = ((strcmp(cmd, ENABLE_EVENT_STR) == 0) || > > + (strcmp(cmd, ENABLE_HIST_STR) == 0)); > > +#else > > + enable = strcmp(cmd, ENABLE_EVENT_STR) == 0; > > +#endif > > trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger); > > > > ret = -ENOMEM; > > @@ -1218,6 +1217,7 @@ event_enable_trigger_func(struct event_command > > *cmd_ops, > > INIT_LIST_HEAD(&trigger_data->list); > > RCU_INIT_POINTER(trigger_data->filter, NULL); > > > > + enable_data->hist = hist; > > enable_data->enable = enable; > > enable_data->file = event_enable_file; > > trigger_data->private_data = enable_data; > > @@ -1295,10 +1295,10 @@ event_enable_trigger_func(struct event_command > > *cmd_ops, > > goto out; > > } > > > > -static int event_enable_register_trigger(char *glob, > > - struct event_trigger_ops *ops, > > - struct event_trigger_data *data, > > - struct trace_event_file *file) > > +int event_enable_register_trigger(char *glob, > > + struct event_trigger_ops *ops, > > + struct event_trigger_data *data, > > + struct trace_event_file *file) > > { > > struct enable_trigger_data *enable_data = data->private_data; > > struct enable_trigger_data *test_enable_data; > > @@ -1308,6 +1308,8 @@ static int event_enable_register_trigger(char *glob, > > list_for_each_entry_rcu(test, &file->triggers, list) { > > test_enable_data = test->private_data; > > if (test_enable_data && > > + (test->cmd_ops->trigger_type == > > + data->cmd_ops->trigger_type) && > > (test_enable_data->file == enable_data->file)) { > > ret = -EEXIST; > > goto out; > > @@ -1333,10 +1335,10 @@ out: > > return ret; > > } > > > > -static void event_enable_unregister_trigger(char *glob, > > - struct event_trigger_ops *ops, > > - struct event_trigger_data *test, > > - struct trace_event_file *file) > > +void event_enable_unregister_trigger(char *glob, > > + struct event_trigger_ops *ops, > > + struct event_trigger_data *test, > > + struct trace_event_file *file) > > { > > struct enable_trigger_data *test_enable_data = test->private_data; > > struct enable_trigger_data *enable_data; > > @@ -1346,6 +1348,8 @@ static void event_enable_unregister_trigger(char > > *glob, > > list_for_each_entry_rcu(data, &file->triggers, list) { > > enable_data = data->private_data; > > if (enable_data && > > + (data->cmd_ops->trigger_type == > > + test->cmd_ops->trigger_type) && > > (enable_data->file == test_enable_data->file)) { > > unregistered = true; > > list_del_rcu(&data->list); > > @@ -1365,8 +1369,12 @@ event_enable_get_trigger_ops(char *cmd, char *param) > > struct event_trigger_ops *ops; > > bool enable; > > > > +#ifdef CONFIG_HIST_TRIGGERS > > + enable = ((strcmp(cmd, ENABLE_EVENT_STR) == 0) || > > + (strcmp(cmd, ENABLE_HIST_STR) == 0)); > > +#else > > enable = strcmp(cmd, ENABLE_EVENT_STR) == 0; > > - > > +#endif > > if (enable) > > ops = param ? &event_enable_count_trigger_ops : > > &event_enable_trigger_ops; > > @@ -1437,6 +1445,7 @@ __init int register_trigger_cmds(void) > > register_trigger_snapshot_cmd(); > > register_trigger_stacktrace_cmd(); > > register_trigger_enable_disable_cmds(); > > + register_trigger_hist_enable_disable_cmds(); > > register_trigger_hist_cmd(); > > > > return 0; > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
