Re: [PATCH v5 2/4] tracing: Add cpu as a key field in histogram

[Daniel Wagner <daniel.wagner@bmw-carit.de>]

Hi Binoy

On 09/02/2016 02:37 PM, Binoy Jayan wrote:
> The field 'cpu' although part of the set of generic fields, is not made
> part of the key fields when mentioned in the trigger command. This hack
> suggested by Daniel marks it as one of the key fields and make it appear
> in the histogram output.

While you are at it, why not adding the COMM as well? I know there is
already the common_pid.execname. With only a few extra lines we get COMM
as well and it allows to write something like

	hist:key=cpu,comm:sort=cpu,comm

for example.

cheers,
daniel



>From cc2f610078ebe9ba8d850cbf602aa529df690cdb Mon Sep 17 00:00:00 2001
From: Daniel Wagner <daniel.wagner@bmw-carit.de>
Date: Wed, 31 Aug 2016 09:37:38 +0200
Subject: [PATCH] tracing: Add hist trigger support for generic fields

Whenever a trace is printed the generic fields (CPU, COMM) are
reconstructed (see trace_print_context()). CPU is taken from the
trace_iterator and COMM is extracted from the savedcmd map (see
__trace_find_cmdline()).

We can't reconstruct this information for hist events. Therefore this
information needs to stored when a new event is added to the hist
buffer.

There is already support for extracting the COMM for the common_pid
field. For this the tracing_map_ops infrasture is used. Unfortantly, we
can't reuse it because it extends an existing hist_field. That means we
first need to add a hist_field before we are able to make reuse of
trace_map_ops.

Furthermore, it is quite easy to extend the current code to support
those two fields by adding hist_field_cpu() and hist_field_comm().

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: Binoy Jayan <binoy.jayan@linaro.org>
---
 kernel/trace/trace.h             |  6 ++++++
 kernel/trace/trace_events.c      | 13 +++++++------
 kernel/trace/trace_events_hist.c | 31 ++++++++++++++++++++++++++++---
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index f783df4..2b8889b 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1288,6 +1288,12 @@ static inline bool is_function_field(struct ftrace_event_field *field)
 	return field->filter_type == FILTER_TRACE_FN;
 }
 
+static inline bool is_generic_field(struct ftrace_event_field *field)
+{
+	return field->filter_type == FILTER_CPU ||
+	       field->filter_type == FILTER_COMM;
+}
+
 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,
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 03c0a48..7d9154d 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -150,9 +150,10 @@ int trace_define_field(struct trace_event_call *call, const char *type,
 }
 EXPORT_SYMBOL_GPL(trace_define_field);
 
-#define __generic_field(type, item, filter_type)			\
+#define __generic_field(type, item, filter_type, size)			\
 	ret = __trace_define_field(&ftrace_generic_fields, #type,	\
-				   #item, 0, 0, is_signed_type(type),	\
+				   #item, 0, size,			\
+				   is_signed_type(type),		\
 				   filter_type);			\
 	if (ret)							\
 		return ret;
@@ -170,10 +171,10 @@ static int trace_define_generic_fields(void)
 {
 	int ret;
 
-	__generic_field(int, CPU, FILTER_CPU);
-	__generic_field(int, cpu, FILTER_CPU);
-	__generic_field(char *, COMM, FILTER_COMM);
-	__generic_field(char *, comm, FILTER_COMM);
+	__generic_field(int, CPU, FILTER_CPU, sizeof(int));
+	__generic_field(int, cpu, FILTER_CPU, sizeof(int));
+	__generic_field(char *, COMM, FILTER_COMM, TASK_COMM_LEN + 1);
+	__generic_field(char *, comm, FILTER_COMM, TASK_COMM_LEN + 1);
 
 	return ret;
 }
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index f3a960e..0b40f22 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -75,6 +75,16 @@ static u64 hist_field_log2(struct hist_field *hist_field, void *event)
 	return (u64) ilog2(roundup_pow_of_two(val));
 }
 
+static u64 hist_field_cpu(struct hist_field *hist_field, void *event)
+{
+	return (u64) smp_processor_id();
+}
+
+static u64 hist_field_comm(struct hist_field *hist_field, void *event)
+{
+	return (u64)(unsigned long)current->comm;
+}
+
 #define DEFINE_HIST_FIELD_FN(type)					\
 static u64 hist_field_##type(struct hist_field *hist_field, void *event)\
 {									\
@@ -119,6 +129,7 @@ enum hist_field_flags {
 	HIST_FIELD_FL_SYSCALL		= 128,
 	HIST_FIELD_FL_STACKTRACE	= 256,
 	HIST_FIELD_FL_LOG2		= 512,
+	HIST_FIELD_FL_COMM		= 1024,
 };
 
 struct hist_trigger_attrs {
@@ -383,6 +394,13 @@ static struct hist_field *create_hist_field(struct ftrace_event_field *field,
 			hist_field->fn = hist_field_dynstring;
 		else
 			hist_field->fn = hist_field_pstring;
+	} else if (is_generic_field(field)) {
+		if (field->filter_type == FILTER_CPU)
+			hist_field->fn = hist_field_cpu;
+		else if (field->filter_type == FILTER_COMM) {
+			flags |= HIST_FIELD_FL_COMM;
+			hist_field->fn = hist_field_comm;
+		}
 	} else {
 		hist_field->fn = select_value_fn(field->size,
 						 field->is_signed);
@@ -748,7 +766,8 @@ static int create_tracing_map_fields(struct hist_trigger_data *hist_data)
 
 			if (hist_field->flags & HIST_FIELD_FL_STACKTRACE)
 				cmp_fn = tracing_map_cmp_none;
-			else if (is_string_field(field))
+			else if (is_string_field(field) ||
+					hist_field->flags & HIST_FIELD_FL_COMM)
 				cmp_fn = tracing_map_cmp_string;
 			else
 				cmp_fn = tracing_map_cmp_num(field->size,
@@ -871,7 +890,8 @@ static inline void add_to_key(char *compound_key, void *key,
 		/* ensure NULL-termination */
 		if (size > key_field->size - 1)
 			size = key_field->size - 1;
-	}
+	} else if (key_field->flags & HIST_FIELD_FL_COMM)
+		size = key_field->field->size;
 
 	memcpy(compound_key + key_field->offset, key, size);
 }
@@ -906,7 +926,8 @@ 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_FL_STRING) {
+			if (key_field->flags & (HIST_FIELD_FL_STRING |
+						HIST_FIELD_FL_COMM)) {
 				key = (void *)(unsigned long)field_contents;
 				use_compound_key = true;
 			} else
@@ -1004,6 +1025,10 @@ hist_trigger_entry_print(struct seq_file *m,
 		} else if (key_field->flags & HIST_FIELD_FL_STRING) {
 			seq_printf(m, "%s: %-50s", key_field->field->name,
 				   (char *)(key + key_field->offset));
+		} else if (key_field->flags & HIST_FIELD_FL_COMM) {
+			seq_printf(m, "%s: %-16s",
+				   key_field->field->name,
+				   (char *)(key + key_field->offset));
 		} else {
 			uval = *(u64 *)(key + key_field->offset);
 			seq_printf(m, "%s: %10llu", key_field->field->name,
-- 
2.7.4