tracing: 'hist' triggers

When a trigger is enabled, the cond flag should be set beforehand, otherwise a trigger that's expecting to process a trace record (e.g. one with post_trigger set) could be invoked without one.

Likewise a trigger's cond flag should be reset after it's disabled, not before.

Signed-off-by Tom Zanussi <[email protected]>

Signed-off-by Daniel Wagner <[email protected]>

---
 kernel/trace/trace_events_trigger.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

kernel/trace/trace_events_trigger.c +6 -4

index 42a4009..4d2f3cc 100644
@@ -543,11 +543,12 @@ static int register_trigger(char *glob, struct event_trigger_ops *ops,
 	list_add_rcu(&data->list, &file->triggers);
 	ret++;
 
+	update_cond_flag(file);
 	if (trace_event_trigger_enable_disable(file, 1) < 0) {
 		list_del_rcu(&data->list);
+		update_cond_flag(file);
 		ret--;
 	}
-	update_cond_flag(file);
 out:
 	return ret;
 }

@@ -575,8 +576,8 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
 		if (data->cmd_ops->trigger_type == test->cmd_ops->trigger_type) {
 			unregistered = true;
 			list_del_rcu(&data->list);
-			update_cond_flag(file);
 			trace_event_trigger_enable_disable(file, 0);
+			update_cond_flag(file);
 			break;
 		}
 	}

@@ -1319,11 +1320,12 @@ static int event_enable_register_trigger(char *glob,
 	list_add_rcu(&data->list, &file->triggers);
 	ret++;
 
+	update_cond_flag(file);
 	if (trace_event_trigger_enable_disable(file, 1) < 0) {
 		list_del_rcu(&data->list);
+		update_cond_flag(file);
 		ret--;
 	}
-	update_cond_flag(file);
 out:
 	return ret;
 }

@@ -1344,8 +1346,8 @@ static void event_enable_unregister_trigger(char *glob,
 		    (enable_data->file == test_enable_data->file)) {
 			unregistered = true;
 			list_del_rcu(&data->list);
-			update_cond_flag(file);
 			trace_event_trigger_enable_disable(file, 0);
+			update_cond_flag(file);
 			break;
 		}
 	}

Make is_string_field() and is_function_field() accessible outside of trace_event_filters.c for other users of ftrace_event_fields.

Signed-off-by Tom Zanussi <[email protected]

---
 kernel/trace/trace.h               | 12 ++++++++++++
 kernel/trace/trace_events_filter.c | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

kernel/trace/trace.h +12 -0

index 4c41fcd..891c5b0 100644
@@ -1050,6 +1050,18 @@ struct filter_pred {
 	unsigned short		right;
 };
 
+static inline bool is_string_field(struct ftrace_event_field *field)
+{
+	return field->filter_type == FILTER_DYN_STRING ||
+	       field->filter_type == FILTER_STATIC_STRING ||
+	       field->filter_type == FILTER_PTR_STRING;
+}
+
+static inline bool is_function_field(struct ftrace_event_field *field)
+{
+	return field->filter_type == FILTER_TRACE_FN;
+}
+
 extern enum regex_type
 filter_parse_regex(char *buff, int len, char **search, int *not);
 extern void print_event_filter(struct trace_event_file *file,

kernel/trace/trace_events_filter.c +0 -12

index 71511eb..245ee5d 100644
@@ -917,18 +917,6 @@ int filter_assign_type(const char *type)
 	return FILTER_OTHER;
 }
 
-static bool is_function_field(struct ftrace_event_field *field)
-{
-	return field->filter_type == FILTER_TRACE_FN;
-}
-
-static bool is_string_field(struct ftrace_event_field *field)
-{
-	return field->filter_type == FILTER_DYN_STRING ||
-	       field->filter_type == FILTER_STATIC_STRING ||
-	       field->filter_type == FILTER_PTR_STRING;
-}
-
 static int is_legal_op(struct ftrace_event_field *field, int op)
 {
 	if (is_string_field(field) &&

Quoted 4 lines · Tom Zanussi

Make is_string_field() and is_function_field() accessible outside of trace_event_filters.c for other users of ftrace_event_fields.

Signed-off-by Tom Zanussi <[email protected]

Reviewed-by Masami Hiramatsu <[email protected]>

BTW, is there any reason why we split this from caller-side change? this short change can be merged into the patch which actual requires this.

Thanks,

Quoted 52 lines

---
 kernel/trace/trace.h               | 12 ++++++++++++
 kernel/trace/trace_events_filter.c | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

kernel/trace/trace.h +12 -0

index 4c41fcd..891c5b0 100644
@@ -1050,6 +1050,18 @@ struct filter_pred {
 	unsigned short		right;
 };
 
+static inline bool is_string_field(struct ftrace_event_field *field)
+{
+	return field->filter_type == FILTER_DYN_STRING ||
+	       field->filter_type == FILTER_STATIC_STRING ||
+	       field->filter_type == FILTER_PTR_STRING;
+}
+
+static inline bool is_function_field(struct ftrace_event_field *field)
+{
+	return field->filter_type == FILTER_TRACE_FN;
+}
+
 extern enum regex_type
 filter_parse_regex(char *buff, int len, char **search, int *not);
 extern void print_event_filter(struct trace_event_file *file,

kernel/trace/trace_events_filter.c +0 -12

index 71511eb..245ee5d 100644
@@ -917,18 +917,6 @@ int filter_assign_type(const char *type)
 	return FILTER_OTHER;
 }
 
-static bool is_function_field(struct ftrace_event_field *field)
-{
-	return field->filter_type == FILTER_TRACE_FN;
-}
-
-static bool is_string_field(struct ftrace_event_field *field)
-{
-	return field->filter_type == FILTER_DYN_STRING ||
-	       field->filter_type == FILTER_STATIC_STRING ||
-	       field->filter_type == FILTER_PTR_STRING;
-}
-
 static int is_legal_op(struct ftrace_event_field *field, int op)
 {
 	if (is_string_field(field) &&

kernel/trace/tracing_map.c line 714

Quoted 8 lines · Tom Zanussi

kernel/trace/tracing_map.c new file +8 -0

+	for (i = 0; i < elt->map->n_fields; i++) {
+		atomic64_set(&dup_elt->fields[i].sum,
+			     atomic64_read(&elt->fields[i].sum));
+		dup_elt->fields[i].cmp_fn = elt->fields[i].cmp_fn;
+	}
+
+	return dup_elt;
+}

So there is a lot of atomic64_{set,read}() in this patch set, what kind of magic properties do you assume they have?

Note that atomic*_{set,read}() are weaker than {WRITE,READ}_ONCE(), so if you're assuming they do that, you're mistaken -- although it is on a TODO list someplace to go fix that.

Quoted 13 lines · Peter Zijlstra

kernel/trace/tracing_map.c line 714

Quoted 8 lines · Tom Zanussi

kernel/trace/tracing_map.c new file +8 -0

+	for (i = 0; i < elt->map->n_fields; i++) {
+		atomic64_set(&dup_elt->fields[i].sum,
+			     atomic64_read(&elt->fields[i].sum));
+		dup_elt->fields[i].cmp_fn = elt->fields[i].cmp_fn;
+	}
+
+	return dup_elt;
+}

So there is a lot of atomic64_{set,read}() in this patch set, what kind of magic properties do you assume they have?

Quoted 3 lines

Note that atomic*_{set,read}() are weaker than {WRITE,READ}_ONCE(), so if you're assuming they do that, you're mistaken -- although it is on a TODO list someplace to go fix that.

Not assuming any magic properties - I just need an atomic 64-bit counter for the sums and that's the API for setting/reading those. When reading a live trace the exact sum you get is kind of arbitrary..

Quoted 14 lines · Tom Zanussi

Quoted 13 lines · Peter Zijlstra

kernel/trace/tracing_map.c line 714

Quoted 8 lines · Tom Zanussi

kernel/trace/tracing_map.c new file +8 -0

+	for (i = 0; i < elt->map->n_fields; i++) {
+		atomic64_set(&dup_elt->fields[i].sum,
+			     atomic64_read(&elt->fields[i].sum));
+		dup_elt->fields[i].cmp_fn = elt->fields[i].cmp_fn;
+	}
+
+	return dup_elt;
+}

So there is a lot of atomic64_{set,read}() in this patch set, what kind of magic properties do you assume they have?

Quoted 7 lines

Quoted 3 lines

Note that atomic*_{set,read}() are weaker than {WRITE,READ}_ONCE(), so if you're assuming they do that, you're mistaken -- although it is on a TODO list someplace to go fix that.

Not assuming any magic properties - I just need an atomic 64-bit counter for the sums and that's the API for setting/reading those. When reading a live trace the exact sum you get is kind of arbitrary..

OK, so atomic64_read() really should provide load consistency (there are a few archs that lack the READ_ONCE() there).

But the atomic64_set() does not provide store consistency, and in the above case it looks like the value you're writing is not exposed yet to concurrency so it doesn't matter how it issues the store.

So as long as you never atomic64_set() a value that is subject to concurrent modification you should be good.

kernel/trace/tracing_map.c line 531

Quoted 2 lines · Tom Zanussi

kernel/trace/tracing_map.c new file +2 -0

+	map->map = kcalloc(map->map_size, sizeof(struct tracing_map_entry),
+			   GFP_KERNEL);

In a later email you state the max map size to be 128k, with a 16 byte struct, that is 2m of memory for this allocation.

Isn't that a tad big for a kmalloc() ?

kernel/trace/trace.c +15 -0

@@ -3782,6 +3785,32 @@ static const char readme_msg[] =
 	"\t   To remove a trigger with a count:\n"
 	"\t     echo '!<trigger>:0 > <system>/<event>/trigger\n"
 	"\t   Filters can be ignored when removing a trigger.\n"
+#ifdef CONFIG_HIST_TRIGGERS
+	"      hist trigger\t- If set, event hits are aggregated into a hash table\n"
+	"\t    Format: hist:keys=<field1>\n"
+	"\t            [:size=#entries]\n"
+	"\t            [if <filter>]\n\n"
+	"\t    When a matching event is hit, an entry is added to a hash\n"
+	"\t    table using the key named.  Keys correspond to fields in the\n"
+	"\t    event's format description.  On an event hit, the value of a\n"
+	"\t    sum called 'hitcount' is incremented, which is simply a count\n"
+	"\t    of event hits.  Keys can be any field.\n\n"
+	"\t    Reading the 'hist' file for the event will dump the hash\n"
+	"\t    table in its entirety to stdout.  Each printed hash table\n"
+	"\t    entry is a simple list of the keys and values comprising the\n"
+	"\t    entry; keys are printed first and are delineated by curly\n"
+	"\t    braces, and are followed by the set of value fields for the\n"

Hi Tom,

Quoted 25 lines · Tom Zanussi

kernel/trace/trace.c +21 -0

@@ -3782,6 +3785,32 @@ static const char readme_msg[] =
 	"\t   To remove a trigger with a count:\n"
 	"\t     echo '!<trigger>:0 > <system>/<event>/trigger\n"
 	"\t   Filters can be ignored when removing a trigger.\n"
+#ifdef CONFIG_HIST_TRIGGERS
+	"      hist trigger\t- If set, event hits are aggregated into a hash table\n"
+	"\t    Format: hist:keys=<field1>\n"
+	"\t            [:size=#entries]\n"
+	"\t            [if <filter>]\n\n"
+	"\t    When a matching event is hit, an entry is added to a hash\n"
+	"\t    table using the key named.  Keys correspond to fields in the\n"
+	"\t    event's format description.  On an event hit, the value of a\n"
+	"\t    sum called 'hitcount' is incremented, which is simply a count\n"
+	"\t    of event hits.  Keys can be any field.\n\n"
+	"\t    Reading the 'hist' file for the event will dump the hash\n"
+	"\t    table in its entirety to stdout.  Each printed hash table\n"
+	"\t    entry is a simple list of the keys and values comprising the\n"
+	"\t    entry; keys are printed first and are delineated by curly\n"
+	"\t    braces, and are followed by the set of value fields for the\n"
+	"\t    entry.  Numeric fields are displayed as base-10 integers.\n"
+	"\t    By default, the size of the hash table is 2048 entries.  The\n"
+	"\t    'size' param can be used to specify more or fewer than that.\n"
+	"\t    The units are in terms of hashtable entries - if a run uses\n"
+	"\t    more entries than specified, the results will show the number\n"
+	"\t    of 'drops', the number of hits that were ignored.  The size\n"

kernel/trace/trace.c static const char readme_msg[] = · line 3812

Quoted 4 lines

kernel/trace/trace.c +4 -0

+	"\t    should be a power of 2 between 128 and 131072 (any non-\n"
+	"\t    power-of-2 number specified will be rounded up).\n\n"
+	"\t    The entries are sorted by 'hitcount' and the sort order is\n"
+	"\t    'ascending'.\n\n"

Hmm, this seems too much about implementation of histogram. Could you shorten this to be a half ?

Thank you,

Allow users to append 'pause' or 'continue' to an existing trigger in order to have it paused or to have a paused trace continue.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
          [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  5 +++++
 kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

kernel/trace/trace.c +5 -0

index 5dd1fc4..547bbc8 100644
@@ -3791,6 +3791,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
+	"\t            [:pause][:continue]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named.  Keys and values\n"

@@ -3821,6 +3822,10 @@ static const char readme_msg[] =
 	"\t    on.  The default if unspecified is 'hitcount' and the.\n"
 	"\t    default sort order is 'ascending'.  To sort in the opposite\n"
 	"\t    direction, append .descending' to the sort key.\n\n"
+	"\t    The 'pause' param can be used to pause an existing hist\n"
+	"\t    trigger or to start a hist trigger but not log any events\n"
+	"\t    until told to do so.  'continue' can be used to start or\n"
+	"\t    restart a paused hist trigger.\n\n"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +23 -3

index 6bf224f..3ae58e7 100644
@@ -78,6 +78,8 @@ struct hist_trigger_attrs {
 	char		*keys_str;
 	char		*vals_str;
 	char		*sort_key_str;
+	bool		pause;
+	bool		cont;
 	unsigned int	map_bits;
 };
 

@@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "sort", strlen("sort")))
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
+		else if (!strncmp(str, "pause", strlen("pause")))
+			attrs->pause = true;
+		else if (!strncmp(str, "continue", strlen("continue")) ||
+			 !strncmp(str, "cont", strlen("cont")))
+			attrs->cont = true;
 		else if (!strncmp(str, "size", strlen("size"))) {
 			int map_bits = parse_map_size(str);
 

@@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
 	if (data->filter_str)
 		seq_printf(m, " if %s", data->filter_str);
 
-	seq_puts(m, " [active]");
+	if (data->paused)
+		seq_puts(m, " [paused]");
+	else
+		seq_puts(m, " [active]");
 
 	seq_putc(m, '\n');
 

@@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)
 {
+	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test;
 	int ret = 0;
 
 	list_for_each_entry_rcu(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			ret = -EEXIST;
+			if (hist_data->attrs->pause)
+				test->paused = true;
+			else if (hist_data->attrs->cont)
+				test->paused = false;
+			else
+				ret = -EEXIST;
 			goto out;
 		}
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)

@@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */
 	if (!ret) {
-		ret = -ENOENT;
+		if (!(attrs->pause || attrs->cont))
+			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)
 		goto out_free;

Hi Tom,

Quoted 7 lines · Tom Zanussi

Allow users to append 'pause' or 'continue' to an existing trigger in order to have it paused or to have a paused trace continue.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

Quoted 4 lines

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
          [ if filter] > event/trigger

Since the only one hist trigger can be set on one event, it seems that we don't need keys for pause/cont/clear (e.g. hist:pause is enough). Anyway, I've found an odd behavior.

[root@localhost tracing]# echo 'hist:keys=parent_pid' > events/sched/sched_process_fork/trigger [root@localhost tracing]# echo 'hist:keys=common_pid:pause' > events/sched/sched_process_fork/trigger [root@localhost tracing]# cat events/sched/sched_process_fork/trigger hist:keys=parent_pid:vals=hitcount:sort=hitcount:size=2048 [paused]

So, the second "pause" command can work with different keys. Moreover, I can remove it with different keys.

[root@localhost tracing]# echo '!hist:keys=child_pid' > events/sched/sched_process_fork/trigger [root@localhost tracing]# cat events/sched/sched_process_fork/trigger
# Available triggers:

# traceon traceoff snapshot stacktrace enable_event disable_event enable_hist disable_hist hist

Thank you,

Quoted 105 lines

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  5 +++++
 kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

kernel/trace/trace.c +5 -0

index 5dd1fc4..547bbc8 100644
@@ -3791,6 +3791,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
+	"\t            [:pause][:continue]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named.  Keys and values\n"

@@ -3821,6 +3822,10 @@ static const char readme_msg[] =
 	"\t    on.  The default if unspecified is 'hitcount' and the.\n"
 	"\t    default sort order is 'ascending'.  To sort in the opposite\n"
 	"\t    direction, append .descending' to the sort key.\n\n"
+	"\t    The 'pause' param can be used to pause an existing hist\n"
+	"\t    trigger or to start a hist trigger but not log any events\n"
+	"\t    until told to do so.  'continue' can be used to start or\n"
+	"\t    restart a paused hist trigger.\n\n"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +23 -3

index 6bf224f..3ae58e7 100644
@@ -78,6 +78,8 @@ struct hist_trigger_attrs {
 	char		*keys_str;
 	char		*vals_str;
 	char		*sort_key_str;
+	bool		pause;
+	bool		cont;
 	unsigned int	map_bits;
 };
 

@@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "sort", strlen("sort")))
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
+		else if (!strncmp(str, "pause", strlen("pause")))
+			attrs->pause = true;
+		else if (!strncmp(str, "continue", strlen("continue")) ||
+			 !strncmp(str, "cont", strlen("cont")))
+			attrs->cont = true;
 		else if (!strncmp(str, "size", strlen("size"))) {
 			int map_bits = parse_map_size(str);
 

@@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
 	if (data->filter_str)
 		seq_printf(m, " if %s", data->filter_str);
 
-	seq_puts(m, " [active]");
+	if (data->paused)
+		seq_puts(m, " [paused]");
+	else
+		seq_puts(m, " [active]");
 
 	seq_putc(m, '\n');
 

@@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)
 {
+	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test;
 	int ret = 0;
 
 	list_for_each_entry_rcu(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			ret = -EEXIST;
+			if (hist_data->attrs->pause)
+				test->paused = true;
+			else if (hist_data->attrs->cont)
+				test->paused = false;
+			else
+				ret = -EEXIST;
 			goto out;
 		}
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)

@@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */
 	if (!ret) {
-		ret = -ENOENT;
+		if (!(attrs->pause || attrs->cont))
+			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)
 		goto out_free;

Hi Masami,

Quoted 10 lines · Masami Hiramatsu

Hi Tom,

Quoted 7 lines · Tom Zanussi

Allow users to append 'pause' or 'continue' to an existing trigger in order to have it paused or to have a paused trace continue.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

Quoted 8 lines

Quoted 4 lines

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
          [ if filter] > event/trigger

Since the only one hist trigger can be set on one event, it seems that we don't need keys for pause/cont/clear (e.g. hist:pause is enough). Anyway, I've found an odd behavior.

Right, because currently there is only one hist trigger per event, the key is ignored, which also accounts for the 'odd' behavior.

But rather than saying it's expected and document it, I think the conclusion from the other comments are that we'll be allowing multiple hist triggers per event, and in that case, they need to be uniquely identifiable by both key and filter.

Quoted 8 lines

[root@localhost tracing]# echo 'hist:keys=parent_pid' > events/sched/sched_process_fork/trigger [root@localhost tracing]# echo 'hist:keys=common_pid:pause' > events/sched/sched_process_fork/trigger [root@localhost tracing]# cat events/sched/sched_process_fork/trigger hist:keys=parent_pid:vals=hitcount:sort=hitcount:size=2048 [paused]

So, the second "pause" command can work with different keys. Moreover, I can remove it with different keys.

Right, this goes away once we have code that deals with multiple histograms per event, which I'll go ahead and implement rather than document the confusion...

Tom

Quoted 104 lines

[root@localhost tracing]# echo '!hist:keys=child_pid' > events/sched/sched_process_fork/trigger [root@localhost tracing]# cat events/sched/sched_process_fork/trigger
# Available triggers:

# traceon traceoff snapshot stacktrace enable_event disable_event enable_hist disable_hist hist

Thank you,

Quoted 97 lines

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  5 +++++
 kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

kernel/trace/trace.c +5 -0

index 5dd1fc4..547bbc8 100644
@@ -3791,6 +3791,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
+	"\t            [:pause][:continue]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named.  Keys and values\n"

@@ -3821,6 +3822,10 @@ static const char readme_msg[] =
 	"\t    on.  The default if unspecified is 'hitcount' and the.\n"
 	"\t    default sort order is 'ascending'.  To sort in the opposite\n"
 	"\t    direction, append .descending' to the sort key.\n\n"
+	"\t    The 'pause' param can be used to pause an existing hist\n"
+	"\t    trigger or to start a hist trigger but not log any events\n"
+	"\t    until told to do so.  'continue' can be used to start or\n"
+	"\t    restart a paused hist trigger.\n\n"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +21 -2

index 6bf224f..3ae58e7 100644
@@ -78,6 +78,8 @@ struct hist_trigger_attrs {
 	char		*keys_str;
 	char		*vals_str;
 	char		*sort_key_str;
+	bool		pause;
+	bool		cont;
 	unsigned int	map_bits;
 };
 

@@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "sort", strlen("sort")))
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
+		else if (!strncmp(str, "pause", strlen("pause")))
+			attrs->pause = true;
+		else if (!strncmp(str, "continue", strlen("continue")) ||
+			 !strncmp(str, "cont", strlen("cont")))
+			attrs->cont = true;
 		else if (!strncmp(str, "size", strlen("size"))) {
 			int map_bits = parse_map_size(str);
 

@@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
 	if (data->filter_str)
 		seq_printf(m, " if %s", data->filter_str);
 
-	seq_puts(m, " [active]");
+	if (data->paused)
+		seq_puts(m, " [paused]");
+	else
+		seq_puts(m, " [active]");
 
 	seq_putc(m, '\n');
 

@@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)
 {
+	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test;
 	int ret = 0;
 
 	list_for_each_entry_rcu(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			ret = -EEXIST;
+			if (hist_data->attrs->pause)
+				test->paused = true;
+			else if (hist_data->attrs->cont)
+				test->paused = false;
+			else
+				ret = -EEXIST;
 			goto out;
 		}
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)

@@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */

Quoted 10 lines

kernel/trace/trace_events_hist.c static int event_hist_trigger_func(struct event_command *cmd_ops, · line 1010

Quoted 8 lines

kernel/trace/trace_events_hist.c +2 -1

 	if (!ret) {
-		ret = -ENOENT;
+		if (!(attrs->pause || attrs->cont))
+			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)
 		goto out_free;

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

Quoted 22 lines · Tom Zanussi

Hi Masami,

Quoted 10 lines · Masami Hiramatsu

Hi Tom,

Quoted 7 lines · Tom Zanussi

Allow users to append 'pause' or 'continue' to an existing trigger in order to have it paused or to have a paused trace continue.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
          [ if filter] > event/trigger

Quoted 8 lines

Quoted 4 lines

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
          [ if filter] > event/trigger

Since the only one hist trigger can be set on one event, it seems that we don't need keys for pause/cont/clear (e.g. hist:pause is enough). Anyway, I've found an odd behavior.

Quoted 7 lines

Right, because currently there is only one hist trigger per event, the key is ignored, which also accounts for the 'odd' behavior.

But rather than saying it's expected and document it, I think the conclusion from the other comments are that we'll be allowing multiple hist triggers per event, and in that case, they need to be uniquely identifiable by both key and filter.

Agreed.

BTW, as I've wrote in another reply, pause/cont/clear commands should be for the "hist" file, not for trigger. So, below command will be better.

# echo pause:name=foo > event/hist
# echo clear:keys=xxx > event/hist

Thank you,

Quoted 137 lines

Quoted 8 lines

[root@localhost tracing]# echo 'hist:keys=parent_pid' > events/sched/sched_process_fork/trigger [root@localhost tracing]# echo 'hist:keys=common_pid:pause' > events/sched/sched_process_fork/trigger [root@localhost tracing]# cat events/sched/sched_process_fork/trigger hist:keys=parent_pid:vals=hitcount:sort=hitcount:size=2048 [paused]

So, the second "pause" command can work with different keys. Moreover, I can remove it with different keys.

Right, this goes away once we have code that deals with multiple histograms per event, which I'll go ahead and implement rather than document the confusion...

Tom

Quoted 104 lines

[root@localhost tracing]# echo '!hist:keys=child_pid' > events/sched/sched_process_fork/trigger [root@localhost tracing]# cat events/sched/sched_process_fork/trigger
# Available triggers:

# traceon traceoff snapshot stacktrace enable_event disable_event enable_hist disable_hist hist

Thank you,

Quoted 97 lines

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  5 +++++
 kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

kernel/trace/trace.c +5 -0

index 5dd1fc4..547bbc8 100644
@@ -3791,6 +3791,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
+	"\t            [:pause][:continue]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named.  Keys and values\n"

@@ -3821,6 +3822,10 @@ static const char readme_msg[] =
 	"\t    on.  The default if unspecified is 'hitcount' and the.\n"
 	"\t    default sort order is 'ascending'.  To sort in the opposite\n"
 	"\t    direction, append .descending' to the sort key.\n\n"
+	"\t    The 'pause' param can be used to pause an existing hist\n"
+	"\t    trigger or to start a hist trigger but not log any events\n"
+	"\t    until told to do so.  'continue' can be used to start or\n"
+	"\t    restart a paused hist trigger.\n\n"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +21 -2

index 6bf224f..3ae58e7 100644
@@ -78,6 +78,8 @@ struct hist_trigger_attrs {
 	char		*keys_str;
 	char		*vals_str;
 	char		*sort_key_str;
+	bool		pause;
+	bool		cont;
 	unsigned int	map_bits;
 };
 

@@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "sort", strlen("sort")))
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
+		else if (!strncmp(str, "pause", strlen("pause")))
+			attrs->pause = true;
+		else if (!strncmp(str, "continue", strlen("continue")) ||
+			 !strncmp(str, "cont", strlen("cont")))
+			attrs->cont = true;
 		else if (!strncmp(str, "size", strlen("size"))) {
 			int map_bits = parse_map_size(str);
 

@@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
 	if (data->filter_str)
 		seq_printf(m, " if %s", data->filter_str);
 
-	seq_puts(m, " [active]");
+	if (data->paused)
+		seq_puts(m, " [paused]");
+	else
+		seq_puts(m, " [active]");
 
 	seq_putc(m, '\n');
 

@@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)
 {
+	struct hist_trigger_data *hist_data = data->private_data;
 	struct event_trigger_data *test;
 	int ret = 0;
 
 	list_for_each_entry_rcu(test, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
-			ret = -EEXIST;
+			if (hist_data->attrs->pause)
+				test->paused = true;
+			else if (hist_data->attrs->cont)
+				test->paused = false;
+			else
+				ret = -EEXIST;
 			goto out;
 		}
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)

@@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */

Quoted 10 lines

kernel/trace/trace_events_hist.c static int event_hist_trigger_func(struct event_command *cmd_ops, · line 1010

Quoted 8 lines

kernel/trace/trace_events_hist.c +2 -1

 	if (!ret) {
-		ret = -ENOENT;
+		if (!(attrs->pause || attrs->cont))
+			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)
 		goto out_free;

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

Allow users to append 'clear' to an existing trigger in order to have the hash table cleared.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause/cont \
           [ if filter] > event/trigger

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause/cont/clear \
          [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  4 +++-
 kernel/trace/trace_events_hist.c | 25 ++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 2 deletions(-)

kernel/trace/trace.c +3 -1

index 547bbc8..27daa28 100644
@@ -3791,7 +3791,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
-	"\t            [:pause][:continue]\n"
+	"\t            [:pause][:continue][:clear]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named.  Keys and values\n"

@@ -3826,6 +3826,8 @@ static const char readme_msg[] =
 	"\t    trigger or to start a hist trigger but not log any events\n"
 	"\t    until told to do so.  'continue' can be used to start or\n"
 	"\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"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +24 -1

index 3ae58e7..d8259fe 100644
@@ -80,6 +80,7 @@ struct hist_trigger_attrs {
 	char		*sort_key_str;
 	bool		pause;
 	bool		cont;
+	bool		clear;
 	unsigned int	map_bits;
 };
 

@@ -188,6 +189,8 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "pause", strlen("pause")))
 			attrs->pause = true;
+		else if (!strncmp(str, "clear", strlen("clear")))
+			attrs->clear = true;
 		else if (!strncmp(str, "continue", strlen("continue")) ||
 			 !strncmp(str, "cont", strlen("cont")))
 			attrs->cont = true;

@@ -888,6 +891,24 @@ static struct event_trigger_ops *event_hist_get_trigger_ops(char *cmd,
 	return &event_hist_trigger_ops;
 }
 
+static void hist_clear(struct event_trigger_data *data)
+{
+	struct hist_trigger_data *hist_data = data->private_data;
+	bool paused;
+
+	paused = data->paused;
+	data->paused = true;
+
+	synchronize_sched();
+
+	tracing_map_clear(hist_data->map);
+
+	atomic64_set(&hist_data->total_hits, 0);
+	atomic64_set(&hist_data->drops, 0);
+
+	data->paused = paused;
+}
+
 static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)

@@ -902,6 +923,8 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				test->paused = true;
 			else if (hist_data->attrs->cont)
 				test->paused = false;
+			else if (hist_data->attrs->clear)
+				hist_clear(test);
 			else
 				ret = -EEXIST;
 			goto out;

@@ -1003,7 +1026,7 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */
 	if (!ret) {
-		if (!(attrs->pause || attrs->cont))
+		if (!(attrs->pause || attrs->cont || attrs->clear))
 			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)

Quoted 11 lines · Tom Zanussi

Allow users to append 'clear' to an existing trigger in order to have the hash table cleared.

This expands the hist trigger syntax from this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause/cont \
           [ if filter] > event/trigger

to this:

    # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause/cont/clear \
          [ if filter] > event/trigger

By the way, since pause/cont/clear is not the trigger but the commands (which executed immediately), I think it should be a write fops of "hist" special file.

e.g. to clear the histogram, write 0 to hist

# echo 0 > event/hist

And pause/cont will be 1 and 2.

# echo 1 > event/hist  <- pause
and
# echo 2 > event/hist  <- continue

What would you think ?

Thanks,

Quoted 94 lines

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  4 +++-
 kernel/trace/trace_events_hist.c | 25 ++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 2 deletions(-)

kernel/trace/trace.c +3 -1

index 547bbc8..27daa28 100644
@@ -3791,7 +3791,7 @@ static const char readme_msg[] =
 	"\t            [:values=<field1[,field2,...]]\n"
 	"\t            [:sort=field1,field2,...]\n"
 	"\t            [:size=#entries]\n"
-	"\t            [:pause][:continue]\n"
+	"\t            [:pause][:continue][:clear]\n"
 	"\t            [if <filter>]\n\n"
 	"\t    When a matching event is hit, an entry is added to a hash\n"
 	"\t    table using the key(s) and value(s) named.  Keys and values\n"

@@ -3826,6 +3826,8 @@ static const char readme_msg[] =
 	"\t    trigger or to start a hist trigger but not log any events\n"
 	"\t    until told to do so.  'continue' can be used to start or\n"
 	"\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"
 #endif
 ;
 

kernel/trace/trace_events_hist.c +24 -1

index 3ae58e7..d8259fe 100644
@@ -80,6 +80,7 @@ struct hist_trigger_attrs {
 	char		*sort_key_str;
 	bool		pause;
 	bool		cont;
+	bool		clear;
 	unsigned int	map_bits;
 };
 

@@ -188,6 +189,8 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
 			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
 		else if (!strncmp(str, "pause", strlen("pause")))
 			attrs->pause = true;
+		else if (!strncmp(str, "clear", strlen("clear")))
+			attrs->clear = true;
 		else if (!strncmp(str, "continue", strlen("continue")) ||
 			 !strncmp(str, "cont", strlen("cont")))
 			attrs->cont = true;

@@ -888,6 +891,24 @@ static struct event_trigger_ops *event_hist_get_trigger_ops(char *cmd,
 	return &event_hist_trigger_ops;
 }
 
+static void hist_clear(struct event_trigger_data *data)
+{
+	struct hist_trigger_data *hist_data = data->private_data;
+	bool paused;
+
+	paused = data->paused;
+	data->paused = true;
+
+	synchronize_sched();
+
+	tracing_map_clear(hist_data->map);
+
+	atomic64_set(&hist_data->total_hits, 0);
+	atomic64_set(&hist_data->drops, 0);
+
+	data->paused = paused;
+}
+
 static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				 struct event_trigger_data *data,
 				 struct trace_event_file *file)

@@ -902,6 +923,8 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 				test->paused = true;
 			else if (hist_data->attrs->cont)
 				test->paused = false;
+			else if (hist_data->attrs->clear)
+				hist_clear(test);
 			else
 				ret = -EEXIST;
 			goto out;

@@ -1003,7 +1026,7 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	 * triggers registered a failure too.
 	 */
 	if (!ret) {
-		if (!(attrs->pause || attrs->cont))
+		if (!(attrs->pause || attrs->cont || attrs->clear))
 			ret = -ENOENT;
 		goto out_free;
 	} else if (ret < 0)

Allow users to have numeric fields displayed as hex values in the output by appending '.hex' to field names:

   # echo hist:keys=aaa,bbb.hex:vals=ccc.hex ... \
              [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  5 +++-
 kernel/trace/trace_events_hist.c | 49 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 4 deletions(-)

kernel/trace/trace.c +4 -1

index 27daa28..14f9472 100644
@@ -3810,7 +3810,10 @@ static const char readme_msg[] =
 	"\t    entry is a simple list of the keys and values comprising the\n"
 	"\t    entry; keys are printed first and are delineated by curly\n"
 	"\t    braces, and are followed by the set of value fields for the\n"
-	"\t    entry.  Numeric fields are displayed as base-10 integers.\n"
+	"\t    entry.  By default, numeric fields are displayed as base-10\n"
+	"\t    integers.  This can be modified by appending any of the\n"
+	"\t    following modifiers to the field name:\n\n"
+	"\t            .hex        display a number as a hex value\n\n"
 	"\t    By default, the size of the hash table is 2048 entries.  The\n"
 	"\t    'size' param can be used to specify more or fewer than that.\n"
 	"\t    The units are in terms of hashtable entries - if a run uses\n"

kernel/trace/trace_events_hist.c +25 -2

index d8259fe..9cc38ee 100644
@@ -72,6 +72,7 @@ enum hist_field_flags {
 	HIST_FIELD_HITCOUNT	= 1,
 	HIST_FIELD_KEY		= 2,
 	HIST_FIELD_STRING	= 4,
+	HIST_FIELD_HEX		= 8,
 };
 
 struct hist_trigger_attrs {

@@ -284,9 +285,20 @@ static int create_val_field(struct hist_trigger_data *hist_data,
 {
 	struct ftrace_event_field *field = NULL;
 	unsigned long flags = 0;
+	char *field_name;
 	int ret = 0;
 
-	field = trace_find_event_field(file->event_call, field_str);
+	field_name = strsep(&field_str, ".");
+	if (field_str) {
+		if (!strcmp(field_str, "hex"))
+			flags |= HIST_FIELD_HEX;
+		else {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+	field = trace_find_event_field(file->event_call, field_name);
 	if (!field) {
 		ret = -EINVAL;
 		goto out;

@@ -349,11 +361,22 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 	struct ftrace_event_field *field = NULL;
 	unsigned long flags = 0;
 	unsigned int key_size;
+	char *field_name;
 	int ret = 0;
 
 	flags |= HIST_FIELD_KEY;
 
-	field = trace_find_event_field(file->event_call, field_str);
+	field_name = strsep(&field_str, ".");
+	if (field_str) {
+		if (!strcmp(field_str, "hex"))
+			flags |= HIST_FIELD_HEX;
+		else {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+	field = trace_find_event_field(file->event_call, field_name);
 	if (!field) {
 		ret = -EINVAL;
 		goto out;

@@ -688,7 +711,11 @@ hist_trigger_entry_print(struct seq_file *m,
 		if (i > hist_data->n_vals)
 			seq_puts(m, ", ");

kernel/trace/trace_events_hist.c +16 -0

@@ -791,9 +818,25 @@ const struct file_operations event_hist_fops = {
 	.release = single_release,
 };
 
+static const char *get_hist_field_flags(struct hist_field *hist_field)
+{
+	const char *flags_str = NULL;
+
+	if (hist_field->flags & HIST_FIELD_HEX)
+		flags_str = "hex";
+
+	return flags_str;
+}
+
 static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
 {
 	seq_printf(m, "%s", hist_field->field->name);
+	if (hist_field->flags) {
+		const char *flags_str = get_hist_field_flags(hist_field);
+
+		if (flags_str)
+			seq_printf(m, ".%s", flags_str);
+	}
 }
 
 static int event_hist_trigger_print(struct seq_file *m,

Signature · 1 line

1.9.3

Hi Tom,

Quoted 90 lines · Tom Zanussi

Allow users to have numeric fields displayed as hex values in the output by appending '.hex' to field names:

   # echo hist:keys=aaa,bbb.hex:vals=ccc.hex ... \
              [ if filter] > event/trigger

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace.c             |  5 +++-
 kernel/trace/trace_events_hist.c | 49 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 4 deletions(-)

kernel/trace/trace.c +4 -1

index 27daa28..14f9472 100644
@@ -3810,7 +3810,10 @@ static const char readme_msg[] =
 	"\t    entry is a simple list of the keys and values comprising the\n"
 	"\t    entry; keys are printed first and are delineated by curly\n"
 	"\t    braces, and are followed by the set of value fields for the\n"
-	"\t    entry.  Numeric fields are displayed as base-10 integers.\n"
+	"\t    entry.  By default, numeric fields are displayed as base-10\n"
+	"\t    integers.  This can be modified by appending any of the\n"
+	"\t    following modifiers to the field name:\n\n"
+	"\t            .hex        display a number as a hex value\n\n"
 	"\t    By default, the size of the hash table is 2048 entries.  The\n"
 	"\t    'size' param can be used to specify more or fewer than that.\n"
 	"\t    The units are in terms of hashtable entries - if a run uses\n"

kernel/trace/trace_events_hist.c +25 -2

index d8259fe..9cc38ee 100644
@@ -72,6 +72,7 @@ enum hist_field_flags {
 	HIST_FIELD_HITCOUNT	= 1,
 	HIST_FIELD_KEY		= 2,
 	HIST_FIELD_STRING	= 4,
+	HIST_FIELD_HEX		= 8,
 };
 
 struct hist_trigger_attrs {

@@ -284,9 +285,20 @@ static int create_val_field(struct hist_trigger_data *hist_data,
 {
 	struct ftrace_event_field *field = NULL;
 	unsigned long flags = 0;
+	char *field_name;
 	int ret = 0;
 
-	field = trace_find_event_field(file->event_call, field_str);
+	field_name = strsep(&field_str, ".");
+	if (field_str) {
+		if (!strcmp(field_str, "hex"))
+			flags |= HIST_FIELD_HEX;
+		else {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+	field = trace_find_event_field(file->event_call, field_name);
 	if (!field) {
 		ret = -EINVAL;
 		goto out;

@@ -349,11 +361,22 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 	struct ftrace_event_field *field = NULL;
 	unsigned long flags = 0;
 	unsigned int key_size;
+	char *field_name;
 	int ret = 0;
 
 	flags |= HIST_FIELD_KEY;
 
-	field = trace_find_event_field(file->event_call, field_str);
+	field_name = strsep(&field_str, ".");
+	if (field_str) {
+		if (!strcmp(field_str, "hex"))
+			flags |= HIST_FIELD_HEX;
+		else {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
+	field = trace_find_event_field(file->event_call, field_name);
 	if (!field) {
 		ret = -EINVAL;
 		goto out;

@@ -688,7 +711,11 @@ hist_trigger_entry_print(struct seq_file *m,
 		if (i > hist_data->n_vals)
 			seq_puts(m, ", ");
 

kernel/trace/trace_events_hist.c hist_trigger_entry_print(struct seq_file *m, · line 721

Quoted 9 lines

kernel/trace/trace_events_hist.c +5 -1

-		if (key_field->flags & HIST_FIELD_STRING) {
+		if (key_field->flags & HIST_FIELD_HEX) {
+			uval = *(u64 *)(key + key_field->offset);
+			seq_printf(m, "%s: %llx",
+				   key_field->field->name, uval);
+		} else if (key_field->flags & HIST_FIELD_STRING) {
 			seq_printf(m, "%s: %-35s", key_field->field->name,
 				   (char *)(key + key_field->offset));
 		} else {

It seems the '.hex' modifier only affects key fields' output..

Thanks,
Namhyung

Quoted 29 lines

kernel/trace/trace_events_hist.c +16 -0

@@ -791,9 +818,25 @@ const struct file_operations event_hist_fops = {
 	.release = single_release,
 };
 
+static const char *get_hist_field_flags(struct hist_field *hist_field)
+{
+	const char *flags_str = NULL;
+
+	if (hist_field->flags & HIST_FIELD_HEX)
+		flags_str = "hex";
+
+	return flags_str;
+}
+
 static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
 {
 	seq_printf(m, "%s", hist_field->field->name);
+	if (hist_field->flags) {
+		const char *flags_str = get_hist_field_flags(hist_field);
+
+		if (flags_str)
+			seq_printf(m, ".%s", flags_str);
+	}
 }
 
 static int event_hist_trigger_print(struct seq_file *m,

Signature · 1 line

1.9.3

If we assume the maximum size for a string field, we don't have to worry about its position. Since we only allow two keys in a compound key and having more than one string key in a given compound key doesn't make much sense anyway, trading a bit of extra space instead of introducing an arbitrary restriction makes more sense.

We also need to use the event field size for static strings when copying the contents, otherwise we get random garbage in the key.

Finally, rearrange the code without changing any functionality by moving the compound key updating code into a separate function.

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 65 +++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 28 deletions(-)

kernel/trace/trace_events_hist.c +2 -2

index 67fffee..4ba7645 100644
@@ -508,8 +508,8 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 			goto out;
 		}
 
-		if (is_string_field(field)) /* should be last key field */
-			key_size = HIST_KEY_SIZE_MAX - key_offset;
+		if (is_string_field(field))
+			key_size = MAX_FILTER_STR_VAL;
 		else
 			key_size = field->size;
 	}

@@ -781,9 +781,36 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data,
 	}
 }
 

kernel/trace/trace_events_hist.c +16 -26

+		if (size > key_field->size - 1)
+			size = key_field->size - 1;
+	}
+
+	memcpy(compound_key + key_field->offset, key, size);
+}
+
 static void event_hist_trigger(struct event_trigger_data *data, void *rec)
 {
 	struct hist_trigger_data *hist_data = data->private_data;
+	bool use_compound_key = (hist_data->n_keys > 1);
 	unsigned long entries[HIST_STACKTRACE_DEPTH];
 	char compound_key[HIST_KEY_SIZE_MAX];
 	struct stack_trace stacktrace;

@@ -798,8 +825,7 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec)
 		return;
 	}
 
-	if (hist_data->n_keys > 1)
-		memset(compound_key, 0, hist_data->key_size);
+	memset(compound_key, 0, hist_data->key_size);
 
 	for (i = hist_data->n_vals; i < hist_data->n_fields; i++) {
 		key_field = hist_data->fields[i];

@@ -816,35 +842,18 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec)
 			key = entries;
 		} else {
 			field_contents = key_field->fn(key_field, rec);
-			if (key_field->flags & HIST_FIELD_STRING)
+			if (key_field->flags & HIST_FIELD_STRING) {
 				key = (void *)field_contents;
-			else
+				use_compound_key = true;
+			} else
 				key = (void *)&field_contents;
-
-			if (hist_data->n_keys > 1) {
-				/* ensure NULL-termination */
-				size_t size = key_field->size - 1;
-
-				if (key_field->flags & HIST_FIELD_STRING) {
-					struct ftrace_event_field *field;
-
-					field = key_field->field;
-					if (field->filter_type == FILTER_DYN_STRING)
-						size = *(u32 *)(rec + field->offset) >> 16;
-					else if (field->filter_type == FILTER_PTR_STRING)
-						size = strlen(key);
-
-					if (size > key_field->size - 1)
-						size = key_field->size - 1;
-				}
-
-				memcpy(compound_key + key_field->offset, key,
-				       size);
-			}
 		}
+
+		if (use_compound_key)
+			add_to_key(compound_key, key, key_field, rec);
 	}
 
-	if (hist_data->n_keys > 1)
+	if (use_compound_key)
 		key = compound_key;
 
 	elt = tracing_map_insert(hist_data->map, key);

Signature · 1 line

1.9.3

Quoted 6 lines · Tom Zanussi

If we assume the maximum size for a string field, we don't have to worry about its position. Since we only allow two keys in a compound key and having more than one string key in a given compound key doesn't make much sense anyway, trading a bit of extra space instead of introducing an arbitrary restriction makes more sense.

Quoted 7 lines

We also need to use the event field size for static strings when copying the contents, otherwise we get random garbage in the key.

Finally, rearrange the code without changing any functionality by moving the compound key updating code into a separate function.

Signed-off-by Tom Zanussi <[email protected]>

Looks good to me. Just a nitpick below..

Quoted 23 lines

---
 kernel/trace/trace_events_hist.c | 65 +++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 28 deletions(-)

kernel/trace/trace_events_hist.c +2 -2

index 67fffee..4ba7645 100644
@@ -508,8 +508,8 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 			goto out;
 		}
 
-		if (is_string_field(field)) /* should be last key field */
-			key_size = HIST_KEY_SIZE_MAX - key_offset;
+		if (is_string_field(field))
+			key_size = MAX_FILTER_STR_VAL;
 		else
 			key_size = field->size;
 	}

@@ -781,9 +781,36 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data,
 	}
 }
 

kernel/trace/trace_events_hist.c static void hist_trigger_elt_update(struct hist_trigger_data *hist_data, · line 793

Quoted 10 lines

kernel/trace/trace_events_hist.c +10 -0

+static inline void add_to_key(char *compound_key, void *key,
+			      struct hist_field *key_field, void *rec)
+{
+	size_t size = key_field->size;
+
+	if (key_field->flags & HIST_FIELD_STRING) {
+		struct ftrace_event_field *field;
+
+		/* ensure NULL-termination */
+		size--;

This is unnecessary since the size value will be updated below anyway. I think it's enough just to move the comment to ...

kernel/trace/trace_events_hist.c static void hist_trigger_elt_update(struct hist_trigger_data *hist_data, · line 802

Quoted 9 lines

kernel/trace/trace_events_hist.c +9 -0

+
+		field = key_field->field;
+		if (field->filter_type == FILTER_DYN_STRING)
+			size = *(u32 *)(rec + field->offset) >> 16;
+		else if (field->filter_type == FILTER_PTR_STRING)
+			size = strlen(key);
+		else if (field->filter_type == FILTER_STATIC_STRING)
+			size = field->size;
+

... here. :)

Quoted 70 lines

kernel/trace/trace_events_hist.c +16 -26

+		if (size > key_field->size - 1)
+			size = key_field->size - 1;
+	}
+
+	memcpy(compound_key + key_field->offset, key, size);
+}
+
 static void event_hist_trigger(struct event_trigger_data *data, void *rec)
 {
 	struct hist_trigger_data *hist_data = data->private_data;
+	bool use_compound_key = (hist_data->n_keys > 1);
 	unsigned long entries[HIST_STACKTRACE_DEPTH];
 	char compound_key[HIST_KEY_SIZE_MAX];
 	struct stack_trace stacktrace;

@@ -798,8 +825,7 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec)
 		return;
 	}
 
-	if (hist_data->n_keys > 1)
-		memset(compound_key, 0, hist_data->key_size);
+	memset(compound_key, 0, hist_data->key_size);
 
 	for (i = hist_data->n_vals; i < hist_data->n_fields; i++) {
 		key_field = hist_data->fields[i];

@@ -816,35 +842,18 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec)
 			key = entries;
 		} else {
 			field_contents = key_field->fn(key_field, rec);
-			if (key_field->flags & HIST_FIELD_STRING)
+			if (key_field->flags & HIST_FIELD_STRING) {
 				key = (void *)field_contents;
-			else
+				use_compound_key = true;
+			} else
 				key = (void *)&field_contents;
-
-			if (hist_data->n_keys > 1) {
-				/* ensure NULL-termination */
-				size_t size = key_field->size - 1;
-
-				if (key_field->flags & HIST_FIELD_STRING) {
-					struct ftrace_event_field *field;
-
-					field = key_field->field;
-					if (field->filter_type == FILTER_DYN_STRING)
-						size = *(u32 *)(rec + field->offset) >> 16;
-					else if (field->filter_type == FILTER_PTR_STRING)
-						size = strlen(key);
-
-					if (size > key_field->size - 1)
-						size = key_field->size - 1;
-				}
-
-				memcpy(compound_key + key_field->offset, key,
-				       size);
-			}
 		}
+
+		if (use_compound_key)
+			add_to_key(compound_key, key, key_field, rec);
 	}
 
-	if (hist_data->n_keys > 1)
+	if (use_compound_key)
 		key = compound_key;
 
 	elt = tracing_map_insert(hist_data->map, key);

Signature · 1 line

1.9.3

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.

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(-)

include/linux/trace_events.h +1 -0

index 0faf48b..0f3ffdd 100644
@@ -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);

kernel/trace/trace.c +11 -0

index 16c64a2..c581750 100644
@@ -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
 ;
 

kernel/trace/trace.h +32 -0

index e6cb781..5e2e3b0 100644
@@ -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

kernel/trace/trace_events_hist.c +115 -0

index 4ba7645..6a43611 100644
@@ -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;
+}

kernel/trace/trace_events_trigger.c +40 -31

index e80f30b..9490d8f 100644
@@ -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;