Hi Tom, On 2015/07/22 1:10, Tom Zanussi wrote: > 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.
At first I must apologies to be confused, I forgot that the event trigger is always enabled (activated) even if the event itself is disabled. And, I meant that the instance of ftrace, ring-buffers and events, so hist trigger already supports instances. e.g. # mkdir instances/foo # echo hist:key=common_pid > instances/foo/events/sched/sched_process_fork/trigger # cat events/sched/sched_process_fork/trigger # Available triggers: # traceon traceoff snapshot stacktrace enable_event disable_event enable_hist disable_hist hist So I've thought if the hist trigger is activated *only when* the event is enabled, we just need the enable/disable_event trigger. > 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 Thanks, but I agree that your current approach, enable_hist, is better than this, since the hist trigger is a special one. > 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 As I've said, I meant the 'instance' as ftrace's instance. However, it seems that the multiple instance should have a unique name (on same ftrace instance :)), so I expect followings. # echo hist:name=foo:keys=xxx if filter1 > event1/trigger # echo hist:name=bar:keys=yyy if filter2 > event1/trigger And hist file shows all histograms on the event. # cat event1/hist # trigger info: hist:name=foo:keys=xxx:vals=hitcount:sort=hitcount:size=2048 [active] .... # trigger info: hist:name=bar:keys=xxx:vals=hitcount:sort=hitcount:size=2048 [active] .... And if we reuse an instance on another event, # echo hist:name=foo:keys=xxx if filter3 > event2/trigger Of course we cannot mix the single key and the compound keys on same event, nor different modifiers nor different types (e.g. string v.s. digit). Thank you, > > 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; >>> >> >> > > > -- Masami HIRAMATSU Linux Technology Research Center, System Productivity Research Dept. Center for Technology Innovation - Systems Engineering Hitachi, Ltd., Research & Development Group E-mail: [email protected] -- 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/
