[PATCH v2 0/9] tracing/boot: Add histogram syntax support in boot-time tracing

[PATCH v2 0/9] tracing/boot: Add histogram syntax support in boot-time tracing

[Masami Hiramatsu]

Hi,

Here is the 2nd version of boot-time tracing to add histogram
syntax extension with a bugfix related hist-trigger.

In this version, I added multi-histograms and multi-hist-actions
support, and update bconf2ftrace.sh to support it. But the
ftrace2bconf.sh is still uses per-event "actions" option because
this "hist" syntax is for programming histogram on the bootconfig
from scratch.
This series also includes per-group/all event enable option
support in bconf2ftrace.sh and ftrce2bconf.sh.

Other changes in v2:
[2/9]: 
       - Cleanup code to add ':' as a prefix for each element
         instead of fixup the last ':'.
       - Fix syntax typo for handler actions.
       - Make pause|continue|clear mutual exclusive.
       - Add __printf() attribute to the append_printf().

'Histogram' options
-------------------
Currently, the boot-time tracing only supports per-event actions
for setting trigger actions. This is enough for short actions
like 'traceon', 'traceoff', 'snapshot' etc. However, it is not good
for the 'hist' trigger action because it is usually too long to write
it in a single string especially if it has an 'onmatch' action.

Here is the new syntax.

    ftrace[.instance.INSTANCE].event.GROUP.EVENT.hist[.N] {
         keys = <KEY>[,...]
         values = <VAL>[,...]
         sort = <SORT-KEY>[,...]
         size = <ENTRIES>
         name = <HISTNAME>
         var { <VAR> = <EXPR> ... }
         pause|continue|clear
         onmax|onchange[.M] { var = <VAR>, <ACTION> [= <PARAM>] }
         onmatch[.M] { event = <EVENT>, <ACTION> [= <PARAM>] }
         filter = <FILTER>
    }
    
Where <ACTION> is one of below;
    
    trace = <EVENT>, <ARG1>[, ...]
    save = <ARG1>[, ...]
    snapshot

And "N" and "M" are digit started strings for multiple histograms
and actions.

For example,

initcall.initcall_finish.actions =
"hist:keys=func:lat=common_timestamp.usecs-$ts0:onmatch(initcall.initcall_start).trace(initcall_latency,func,$lat)"

This can be written as below;

initcall.initcall_finish.hist {
    keys = func
    var.lat = common_timestamp.usecs-$ts0
    onmatch {
        event = initcall.initcall_start
        trace = initcall_latency, func, $lat
    }
}

Also, you can add comments for each options.


TODO
====
- Need to expand ktest testcase for checking this syntax.

Thank you,

---

Masami Hiramatsu (9):
      tracing/boot: Fix a hist trigger dependency for boot time tracing
      tracing/boot: Add per-event histogram action options
      tracing/boot: Support multiple handlers for per-event histogram
      tracing/boot: Support multiple histograms for each event
      tracing/boot: Show correct histogram error command
      Documentation: tracing: Add histogram syntax to boot-time tracing
      tools/bootconfig: Support per-group/all event enabling option
      tools/bootconfig: Add histogram syntax support to bconf2ftrace.sh
      tools/bootconfig: Use per-group/all enable option in ftrace2bconf script


 tools/bootconfig/scripts/bconf2ftrace.sh |   97 ++++++++++++++++++++++++++++++
 tools/bootconfig/scripts/ftrace2bconf.sh |   24 ++++++-
 tools/bootconfig/scripts/xbc.sh          |    4 +
 3 files changed, 117 insertions(+), 8 deletions(-)

-- 
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH v2 1/9] tracing/boot: Fix a hist trigger dependency for boot time tracing

[Masami Hiramatsu]

Fixes a build error when CONFIG_HIST_TRIGGERS=n with boot-time
tracing. Since the trigger_process_regex() is defined only
when CONFIG_HIST_TRIGGERS=y, if it is disabled, the 'actions'
event option also must be disabled.

Fixes: 81a59555ff15 ("tracing/boot: Add per-event settings")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 0 files changed

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index 94ef2d099e32..e6dc9269ad75 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -204,13 +204,14 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
 		else if (apply_event_filter(file, buf) < 0)
 			pr_err("Failed to apply filter: %s\n", buf);
 	}
-
+#ifdef CONFIG_HIST_TRIGGERS
 	xbc_node_for_each_array_value(enode, "actions", anode, p) {
 		if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
 			pr_err("action string is too long: %s\n", p);
 		else if (trigger_process_regex(file, buf) < 0)
 			pr_err("Failed to apply an action: %s\n", buf);
 	}
+#endif
 
 	if (xbc_node_find_value(enode, "enable", NULL)) {
 		if (trace_event_enable_disable(file, 1, 0) < 0)

[PATCH v2 2/9] tracing/boot: Add per-event histogram action options

[Masami Hiramatsu]

Add a hist-trigger action syntax support to boot-time tracing.
Currently, boot-time tracing supports per-event actions as option
strings. However, for the histogram action, it has a special syntax
and usually needs a long action definition.
To make it readable and fit to the bootconfig syntax, this introduces
a new options for histogram.

Here are the histogram action options for boot-time tracing.

ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist {
     keys = <KEY>[,...]
     values = <VAL>[,...]
     sort = <SORT-KEY>[,...]
     size = <ENTRIES>
     name = <HISTNAME>
     var { <VAR> = <EXPR> ... }
     pause|continue|clear
     onmax|onchange { var = <VAR>; <ACTION> [= <PARAM>] }
     onmatch { event = <EVENT>; <ACTION> [= <PARAM>] }
     filter = <FILTER>
}

Where <ACTION> is one of below;

     trace = <EVENT>, <ARG1>[, ...]
     save = <ARG1>[, ...]
     snapshot

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
   - Cleanup code to add ':' as a prefix for each element
     instead of fixup the last ':'.
   - Fix syntax typo for handler actions.
   - Make pause|continue|clear mutual exclusive.
   - Add __printf() attribute to the append_printf().
---
 0 files changed

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index e6dc9269ad75..aaaf8b8ed3c9 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -171,6 +171,202 @@ trace_boot_add_synth_event(struct xbc_node *node, const char *event)
 }
 #endif
 
+#ifdef CONFIG_HIST_TRIGGERS
+static int __init __printf(3, 4)
+append_printf(char **bufp, char *end, const char *fmt, ...)
+{
+	va_list args;
+	int ret;
+
+	if (*bufp == end)
+		return -ENOSPC;
+
+	va_start(args, fmt);
+	ret = vsnprintf(*bufp, end - *bufp, fmt, args);
+	if (ret < end - *bufp) {
+		*bufp += ret;
+	} else {
+		*bufp = end;
+		ret = -ERANGE;
+	}
+	va_end(args);
+
+	return ret;
+}
+
+static int __init
+trace_boot_hist_add_array(struct xbc_node *hnode, char **bufp,
+			  char *end, const char *key)
+{
+	struct xbc_node *knode, *anode;
+	const char *p;
+	char sep;
+
+	knode = xbc_node_find_child(hnode, key);
+	if (knode) {
+		anode = xbc_node_get_child(knode);
+		if (!anode) {
+			pr_err("hist.%s requires value(s).\n", key);
+			return -EINVAL;
+		}
+
+		append_printf(bufp, end, ":%s", key);
+		sep = '=';
+		xbc_array_for_each_value(anode, p) {
+			append_printf(bufp, end, "%c%s", sep, p);
+			if (sep == '=')
+				sep = ',';
+		}
+	} else
+		return -ENOENT;
+
+	return 0;
+}
+
+static int __init
+trace_boot_hist_add_handler(struct xbc_node *hnode, char **bufp,
+			    char *end, const char *param)
+{
+	struct xbc_node *knode, *anode;
+	const char *p;
+	char sep;
+
+	/* Compose 'handler' parameter */
+	p = xbc_node_find_value(hnode, param, NULL);
+	if (!p) {
+		pr_err("hist.%s requires '%s' option.\n",
+		       xbc_node_get_data(hnode), param);
+		return -EINVAL;
+	}
+	append_printf(bufp, end, ":%s(%s)", xbc_node_get_data(hnode), p);
+
+	/* Compose 'action' parameter */
+	knode = xbc_node_find_child(hnode, "trace");
+	if (!knode)
+		knode = xbc_node_find_child(hnode, "save");
+
+	if (knode) {
+		anode = xbc_node_get_child(knode);
+		if (!anode || !xbc_node_is_value(anode)) {
+			pr_err("hist.%s.%s requires value(s).\n",
+			       xbc_node_get_data(hnode),
+			       xbc_node_get_data(knode));
+			return -EINVAL;
+		}
+
+		append_printf(bufp, end, ".%s", xbc_node_get_data(knode));
+		sep = '(';
+		xbc_array_for_each_value(anode, p) {
+			append_printf(bufp, end, "%c%s", sep, p);
+			if (sep == '(')
+				sep = ',';
+		}
+		append_printf(bufp, end, ")");
+	} else if (xbc_node_find_child(hnode, "snapshot")) {
+		append_printf(bufp, end, ".snapshot()");
+	} else {
+		pr_err("hist.%s requires an action.\n",
+		       xbc_node_get_data(hnode));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+ * Histogram boottime tracing syntax.
+ *
+ * ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist {
+ *	keys = <KEY>[,...]
+ *	values = <VAL>[,...]
+ *	sort = <SORT-KEY>[,...]
+ *	size = <ENTRIES>
+ *	name = <HISTNAME>
+ *	var { <VAR> = <EXPR> ... }
+ *	pause|continue|clear
+ *	onmax|onchange { var = <VAR>; <ACTION> [= <PARAM>] }
+ *	onmatch { event = <EVENT>; <ACTION> [= <PARAM>] }
+ *	filter = <FILTER>
+ * }
+ *
+ * Where <ACTION> are;
+ *
+ *	trace = <EVENT>, <ARG1>[, ...]
+ *	save = <ARG1>[, ...]
+ *	snapshot
+ */
+static int __init
+trace_boot_compose_hist_cmd(struct xbc_node *hnode, char *buf, size_t size)
+{
+	struct xbc_node *node, *knode;
+	char *end = buf + size;
+	const char *p;
+	int ret = 0;
+
+	append_printf(&buf, end, "hist");
+
+	ret = trace_boot_hist_add_array(hnode, &buf, end, "keys");
+	if (ret < 0) {
+		if (ret == -ENOENT)
+			pr_err("hist requires keys.\n");
+		return -EINVAL;
+	}
+
+	ret = trace_boot_hist_add_array(hnode, &buf, end, "values");
+	if (ret == -EINVAL)
+		return ret;
+	ret = trace_boot_hist_add_array(hnode, &buf, end, "sort");
+	if (ret == -EINVAL)
+		return ret;
+
+	p = xbc_node_find_value(hnode, "size", NULL);
+	if (p)
+		append_printf(&buf, end, ":size=%s", p);
+
+	p = xbc_node_find_value(hnode, "name", NULL);
+	if (p)
+		append_printf(&buf, end, ":name=%s", p);
+
+	node = xbc_node_find_child(hnode, "var");
+	if (node) {
+		xbc_node_for_each_key_value(node, knode, p) {
+			append_printf(&buf, end, ":%s=%s",
+				      xbc_node_get_data(knode), p);
+		}
+	}
+
+	/* Histogram control attributes (mutual exclusive) */
+	if (xbc_node_find_child(hnode, "pause"))
+		append_printf(&buf, end, ":pause");
+	else if (xbc_node_find_child(hnode, "continue"))
+		append_printf(&buf, end, ":continue");
+	else if (xbc_node_find_child(hnode, "clear"))
+		append_printf(&buf, end, ":clear");
+
+	/* Histogram handler and actions */
+	node = xbc_node_find_child(hnode, "onmax");
+	if (node && trace_boot_hist_add_handler(node, &buf, end, "var") < 0)
+		return -EINVAL;
+	node = xbc_node_find_child(hnode, "onchange");
+	if (node && trace_boot_hist_add_handler(node, &buf, end, "var") < 0)
+		return -EINVAL;
+	node = xbc_node_find_child(hnode, "onmatch");
+	if (node && trace_boot_hist_add_handler(node, &buf, end, "event") < 0)
+		return -EINVAL;
+
+	p = xbc_node_find_value(hnode, "filter", NULL);
+	if (p)
+		append_printf(&buf, end, " if %s", p);
+
+	if (buf == end) {
+		pr_err("hist exceeds the max command length.\n");
+		return -E2BIG;
+	}
+
+	return 0;
+}
+#endif
+
 static void __init
 trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
 			  struct xbc_node *enode)
@@ -211,6 +407,12 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
 		else if (trigger_process_regex(file, buf) < 0)
 			pr_err("Failed to apply an action: %s\n", buf);
 	}
+	anode = xbc_node_find_child(enode, "hist");
+	if (anode &&
+	    trace_boot_compose_hist_cmd(anode, buf, ARRAY_SIZE(buf)) == 0) {
+		if (trigger_process_regex(file, buf) < 0)
+			pr_err("Failed to apply hist trigger: %s\n", buf);
+	}
 #endif
 
 	if (xbc_node_find_value(enode, "enable", NULL)) {

[PATCH v2 3/9] tracing/boot: Support multiple handlers for per-event histogram

[Masami Hiramatsu]

Support multiple handlers for per-event histogram in boot-time tracing.
Since the histogram can register multiple same handler-actions with
different parameters, this expands the syntax to support such cases.

With this update, the 'onmax', 'onchange' and 'onmatch' handler subkeys
under per-event histogram option will take a number subkeys optionally
as below. (see [.N])

ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist {
     onmax|onchange[.N] { var = <VAR>; <ACTION> [= <PARAM>] }
     onmatch[.N] { event = <EVENT>; <ACTION> [= <PARAM>] }
}

The 'N' must be a digit (or digit started word).

Thus user can add several handler-actions to the histogram,
for example,

ftrace.event.SOMEGROUP.SOMEEVENT.hist {
   keys = SOME_ID; lat = common_timestamp.usecs-$ts0
   onmatch.1 {
	event = GROUP1.STARTEVENT1
	trace = latency_event, SOME_ID, $lat
   }
   onmatch.2 {
	event = GROUP2.STARTEVENT2
	trace = latency_event, SOME_ID, $lat
   }
}

Then, it can trace the elapsed time from GROUP1.STARTEVENT1 to
SOMEGROUP.SOMEEVENT, and from GROUP2.STARTEVENT2 to
SOMEGROUP.SOMEEVENT with SOME_ID key.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 0 files changed

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index aaaf8b8ed3c9..0db0775d37af 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -224,8 +224,9 @@ trace_boot_hist_add_array(struct xbc_node *hnode, char **bufp,
 }
 
 static int __init
-trace_boot_hist_add_handler(struct xbc_node *hnode, char **bufp,
-			    char *end, const char *param)
+trace_boot_hist_add_one_handler(struct xbc_node *hnode, char **bufp,
+				char *end, const char *handler,
+				const char *param)
 {
 	struct xbc_node *knode, *anode;
 	const char *p;
@@ -238,7 +239,7 @@ trace_boot_hist_add_handler(struct xbc_node *hnode, char **bufp,
 		       xbc_node_get_data(hnode), param);
 		return -EINVAL;
 	}
-	append_printf(bufp, end, ":%s(%s)", xbc_node_get_data(hnode), p);
+	append_printf(bufp, end, ":%s(%s)", handler, p);
 
 	/* Compose 'action' parameter */
 	knode = xbc_node_find_child(hnode, "trace");
@@ -273,6 +274,32 @@ trace_boot_hist_add_handler(struct xbc_node *hnode, char **bufp,
 	return 0;
 }
 
+static int __init
+trace_boot_hist_add_handlers(struct xbc_node *hnode, char **bufp,
+			     char *end, const char *param)
+{
+	struct xbc_node *node;
+	const char *p, *handler;
+	int ret;
+
+	handler = xbc_node_get_data(hnode);
+
+	xbc_node_for_each_subkey(hnode, node) {
+		p = xbc_node_get_data(node);
+		if (!isdigit(p[0]))
+			continue;
+		/* All digit started node should be instances. */
+		ret = trace_boot_hist_add_one_handler(node, bufp, end, handler, param);
+		if (ret < 0)
+			break;
+	}
+
+	if (xbc_node_find_child(hnode, param))
+		ret = trace_boot_hist_add_one_handler(hnode, bufp, end, handler, param);
+
+	return ret;
+}
+
 /*
  * Histogram boottime tracing syntax.
  *
@@ -284,8 +311,8 @@ trace_boot_hist_add_handler(struct xbc_node *hnode, char **bufp,
  *	name = <HISTNAME>
  *	var { <VAR> = <EXPR> ... }
  *	pause|continue|clear
- *	onmax|onchange { var = <VAR>; <ACTION> [= <PARAM>] }
- *	onmatch { event = <EVENT>; <ACTION> [= <PARAM>] }
+ *	onmax|onchange[.N] { var = <VAR>; <ACTION> [= <PARAM>] }
+ *	onmatch[.N] { event = <EVENT>; <ACTION> [= <PARAM>] }
  *	filter = <FILTER>
  * }
  *
@@ -345,13 +372,13 @@ trace_boot_compose_hist_cmd(struct xbc_node *hnode, char *buf, size_t size)
 
 	/* Histogram handler and actions */
 	node = xbc_node_find_child(hnode, "onmax");
-	if (node && trace_boot_hist_add_handler(node, &buf, end, "var") < 0)
+	if (node && trace_boot_hist_add_handlers(node, &buf, end, "var") < 0)
 		return -EINVAL;
 	node = xbc_node_find_child(hnode, "onchange");
-	if (node && trace_boot_hist_add_handler(node, &buf, end, "var") < 0)
+	if (node && trace_boot_hist_add_handlers(node, &buf, end, "var") < 0)
 		return -EINVAL;
 	node = xbc_node_find_child(hnode, "onmatch");
-	if (node && trace_boot_hist_add_handler(node, &buf, end, "event") < 0)
+	if (node && trace_boot_hist_add_handlers(node, &buf, end, "event") < 0)
 		return -EINVAL;
 
 	p = xbc_node_find_value(hnode, "filter", NULL);

[PATCH v2 4/9] tracing/boot: Support multiple histograms for each event

[Masami Hiramatsu]

Add multiple histograms support for each event. This allows
user to set multiple histograms to an event.

ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist[.N] {
...
}

The 'N' is a digit started string and it can be omitted
for the default histogram.

For example, multiple hist triggers example in the
Documentation/trace/histogram.rst can be written as below;

ftrace.event.net.netif_receive_skb.hist {
	1 {
		keys = skbaddr.hex
		values = len
		filter = len < 0
	}
	2 {
		keys = skbaddr.hex
		values = len
		filter = len > 4096
	}
	3 {
		keys = skbaddr.hex
		values = len
		filter = len == 256
	}
	4 {
		keys = skbaddr.hex
		values = len
	}
	5 {
		keys = len
		values = common_preempt_count
	}
}

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 0 files changed

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index 0db0775d37af..1f4b80f6fcb6 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -303,7 +303,7 @@ trace_boot_hist_add_handlers(struct xbc_node *hnode, char **bufp,
 /*
  * Histogram boottime tracing syntax.
  *
- * ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist {
+ * ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist[.N] {
  *	keys = <KEY>[,...]
  *	values = <VAL>[,...]
  *	sort = <SORT-KEY>[,...]
@@ -392,6 +392,32 @@ trace_boot_compose_hist_cmd(struct xbc_node *hnode, char *buf, size_t size)
 
 	return 0;
 }
+
+static void __init
+trace_boot_init_histograms(struct trace_event_file *file,
+			   struct xbc_node *hnode, char *buf, size_t size)
+{
+	struct xbc_node *node;
+	const char *p;
+
+	xbc_node_for_each_subkey(hnode, node) {
+		p = xbc_node_get_data(node);
+		if (!isdigit(p[0]))
+			continue;
+		/* All digit started node should be instances. */
+		if (trace_boot_compose_hist_cmd(node, buf, size) == 0) {
+			if (trigger_process_regex(file, buf) < 0)
+				pr_err("Failed to apply hist trigger: %s\n", buf);
+		}
+	}
+
+	if (xbc_node_find_child(hnode, "keys")) {
+		if (trace_boot_compose_hist_cmd(hnode, buf, size) == 0)
+			if (trigger_process_regex(file, buf) < 0)
+				pr_err("Failed to apply hist trigger: %s\n", buf);
+	}
+
+}
 #endif
 
 static void __init
@@ -435,11 +461,8 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
 			pr_err("Failed to apply an action: %s\n", buf);
 	}
 	anode = xbc_node_find_child(enode, "hist");
-	if (anode &&
-	    trace_boot_compose_hist_cmd(anode, buf, ARRAY_SIZE(buf)) == 0) {
-		if (trigger_process_regex(file, buf) < 0)
-			pr_err("Failed to apply hist trigger: %s\n", buf);
-	}
+	if (anode)
+		trace_boot_init_histograms(file, anode, buf, ARRAY_SIZE(buf));
 #endif
 
 	if (xbc_node_find_value(enode, "enable", NULL)) {

[PATCH v2 5/9] tracing/boot: Show correct histogram error command

[Masami Hiramatsu]

Since trigger_process_regex() modifies given trigger actions
while parsing, the error message couldn't show what command
was passed to the trigger_process_regex() when it returns
an error.

To fix that, show the backed up trigger action command
instead of parsed buffer.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 0 files changed

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index 1f4b80f6fcb6..0fa7167bb1e5 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -399,6 +399,7 @@ trace_boot_init_histograms(struct trace_event_file *file,
 {
 	struct xbc_node *node;
 	const char *p;
+	char *tmp;
 
 	xbc_node_for_each_subkey(hnode, node) {
 		p = xbc_node_get_data(node);
@@ -406,15 +407,19 @@ trace_boot_init_histograms(struct trace_event_file *file,
 			continue;
 		/* All digit started node should be instances. */
 		if (trace_boot_compose_hist_cmd(node, buf, size) == 0) {
+			tmp = kstrdup(buf, GFP_KERNEL);
 			if (trigger_process_regex(file, buf) < 0)
-				pr_err("Failed to apply hist trigger: %s\n", buf);
+				pr_err("Failed to apply hist trigger: %s\n", tmp);
+			kfree(tmp);
 		}
 	}
 
 	if (xbc_node_find_child(hnode, "keys")) {
 		if (trace_boot_compose_hist_cmd(hnode, buf, size) == 0)
+			tmp = kstrdup(buf, GFP_KERNEL);
 			if (trigger_process_regex(file, buf) < 0)
-				pr_err("Failed to apply hist trigger: %s\n", buf);
+				pr_err("Failed to apply hist trigger: %s\n", tmp);
+			kfree(tmp);
 	}
 
 }
@@ -458,7 +463,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
 		if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
 			pr_err("action string is too long: %s\n", p);
 		else if (trigger_process_regex(file, buf) < 0)
-			pr_err("Failed to apply an action: %s\n", buf);
+			pr_err("Failed to apply an action: %s\n", p);
 	}
 	anode = xbc_node_find_child(enode, "hist");
 	if (anode)

[PATCH v2 6/9] Documentation: tracing: Add histogram syntax to boot-time tracing

[Masami Hiramatsu]

Add the documentation about histogram syntax in boot-time tracing.
This will allow user to write the histogram setting in a structured
parameters.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
  - Update for the multiple histogram and the multiple handler.
---
 0 files changed

diff --git a/Documentation/trace/boottime-trace.rst b/Documentation/trace/boottime-trace.rst
index 8053898cfeb4..74e04210fda8 100644
--- a/Documentation/trace/boottime-trace.rst
+++ b/Documentation/trace/boottime-trace.rst
@@ -125,6 +125,71 @@ Note that kprobe and synthetic event definitions can be written under
 instance node, but those are also visible from other instances. So please
 take care for event name conflict.
 
+Ftrace Histogram Options
+------------------------
+
+Since it is too long to write a histogram action as a string for per-event
+action option, there are tree-style options under per-event 'hist' subkey
+for the histogram actions. For the detail of the each parameter,
+please read the event histogram document [3]_.
+
+.. [3] See :ref:`Documentation/trace/histogram.rst <histogram>`
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]keys = KEY1[, KEY2[...]]
+  Set histogram key parameters. (Mandatory)
+  The 'N' is a digit string for the multiple histogram. You can omit it
+  if there is one histogram on the event.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]values = VAL1[, VAL2[...]]
+  Set histogram value parameters.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]sort = SORT1[, SORT2[...]]
+  Set histogram sort parameter options.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]size = NR_ENTRIES
+  Set histogram size (number of entries).
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]name = NAME
+  Set histogram name.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]var.VARIABLE = EXPR
+  Define a new VARIABLE by EXPR expression.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]<pause|continue|clear>
+  Set histogram control parameter. You can set one of them.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]onmatch.[M.]event = GROUP.EVENT
+  Set histogram 'onmatch' handler matching event parameter.
+  The 'M' is a digit string for the multiple 'onmatch' handler. You can omit it
+  if there is one 'onmatch' handler on this histogram.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]onmatch.[M.]trace = EVENT[, ARG1[...]]
+  Set histogram 'trace' action for 'onmatch'.
+  EVENT must be a synthetic event name, and ARG1... are parameters
+  for that event. Mandatory if 'onmatch.event' option is set.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]onmax.[M.]var = VAR
+  Set histogram 'onmax' handler variable parameter.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]onchange.[M.]var = VAR
+  Set histogram 'onchange' handler variable parameter.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]<onmax|onchange>.[M.]save = ARG1[, ARG2[...]]
+  Set histogram 'save' action parameters for 'onmax' or 'onchange' handler.
+  This option or below 'snapshot' option is mandatory if 'onmax.var' or
+  'onchange.var' option is set.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.[N.]<onmax|onchange>.[M.]snapshot
+  Set histogram 'snapshot' action for 'onmax' or 'onchange' handler.
+  This option or above 'save' option is mandatory if 'onmax.var' or
+  'onchange.var' option is set.
+
+ftrace.[instance.INSTANCE.]event.GROUP.EVENT.hist.filter = FILTER_EXPR
+  Set histogram filter expression. You don't need 'if' in the FILTER_EXPR.
+
+Note that this 'hist' option can conflict with the per-event 'actions'
+option if the 'actions' option has a histogram action.
+
 
 When to Start
 =============
@@ -159,13 +224,23 @@ below::
         }
         synthetic.initcall_latency {
                 fields = "unsigned long func", "u64 lat"
-                actions = "hist:keys=func.sym,lat:vals=lat:sort=lat"
+                hist {
+                        keys = func.sym, lat
+                        values = lat
+                        sort = lat
+                }
         }
-        initcall.initcall_start {
-                actions = "hist:keys=func:ts0=common_timestamp.usecs"
+        initcall.initcall_start.hist {
+                keys = func
+                var.ts0 = common_timestamp.usecs
         }
-        initcall.initcall_finish {
-                actions = "hist:keys=func:lat=common_timestamp.usecs-$ts0:onmatch(initcall.initcall_start).initcall_latency(func,$lat)"
+        initcall.initcall_finish.hist {
+                keys = func
+                var.lat = common_timestamp.usecs-$ts0
+                onmatch {
+                        event = initcall.initcall_start
+                        trace = initcall_latency, func, $lat
+                }
         }
   }
 

[PATCH v2 7/9] tools/bootconfig: Support per-group/all event enabling option

[Masami Hiramatsu]

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/scripts/bconf2ftrace.sh |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/bootconfig/scripts/bconf2ftrace.sh b/tools/bootconfig/scripts/bconf2ftrace.sh
index feb30c2c7881..651049c782c0 100755
--- a/tools/bootconfig/scripts/bconf2ftrace.sh
+++ b/tools/bootconfig/scripts/bconf2ftrace.sh
@@ -101,6 +101,12 @@ setup_event() { # prefix group event [instance]
 	else
 		eventdir="$TRACEFS/events/$2/$3"
 	fi
+	# group enable
+	if [ "$3" = "enable" ]; then
+		run_cmd "echo 1 > ${eventdir}"
+		return
+	fi
+
 	case $2 in
 	kprobes)
 		xbc_get_val ${branch}.probes | while read line; do
@@ -127,6 +133,13 @@ setup_events() { # prefix("ftrace" or "ftrace.instance.INSTANCE") [instance]
 			setup_event $prefix ${grpev%.*} ${grpev#*.} $2
 		done
 	fi
+	if xbc_has_branch ${1}.event.enable; then
+		if [ "$2" ]; then
+			run_cmd "echo 1 > $TRACEFS/instances/$2/events/enable"
+		else
+			run_cmd "echo 1 > $TRACEFS/events/enable"
+		fi
+	fi
 }
 
 size2kb() { # size[KB|MB]

[PATCH v2 8/9] tools/bootconfig: Add histogram syntax support to bconf2ftrace.sh

[Masami Hiramatsu]

Add histogram syntax support to bconf2ftrace.sh script.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/scripts/bconf2ftrace.sh |   84 ++++++++++++++++++++++++++++++
 tools/bootconfig/scripts/xbc.sh          |    4 +
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/tools/bootconfig/scripts/bconf2ftrace.sh b/tools/bootconfig/scripts/bconf2ftrace.sh
index 651049c782c0..43b65a199010 100755
--- a/tools/bootconfig/scripts/bconf2ftrace.sh
+++ b/tools/bootconfig/scripts/bconf2ftrace.sh
@@ -94,6 +94,88 @@ compose_synth() { # event_name branch
 	xbc_get_val $2 | while read field; do echo -n "$field; "; done
 }
 
+print_hist_array() { # prefix key
+	__sep="="
+	if xbc_has_key ${1}.${2}; then
+		echo -n ":$2"
+		xbc_get_val ${1}.${2} | while read field; do
+			echo -n "$__sep$field"; __sep=","
+		done
+	fi
+}
+
+print_hist_action_array() { # prefix key
+	__sep="("
+	echo -n ".$2"
+	xbc_get_val ${1}.${2} | while read field; do
+		echo -n "$__sep$field"; __sep=","
+	done
+	echo -n ")"
+}
+
+print_hist_one_action() { # prefix handler param
+	echo -n ":${2}("`xbc_get_val ${1}.${3}`")"
+	if xbc_has_key "${1}.trace"; then
+		print_hist_action_array ${1} "trace"
+	elif xbc_has_key "${1}.save"; then
+		print_hist_action_array ${1} "save"
+	elif xbc_has_key "${1}.snapshot"; then
+		echo -n ".snapshot()"
+	fi
+}
+
+print_hist_actions() { # prefix handler param
+	for __hdr in `xbc_subkeys ${1}.${2} 1 ".[0-9]"`; do
+		print_hist_one_action ${1}.${2}.$__hdr ${2} ${3}
+	done
+	if xbc_has_key ${1}.${2}.${3} ; then
+		print_hist_one_action ${1}.${2} ${2} ${3}
+	fi
+}
+
+print_one_histogram() { # prefix
+	echo -n "hist"
+	print_hist_array $1 "keys"
+	print_hist_array $1 "values"
+	print_hist_array $1 "sort"
+	if xbc_has_key "${1}.size"; then
+		echo -n ":size="`xbc_get_val ${1}.size`
+	fi
+	if xbc_has_key "${1}.name"; then
+		echo -n ":name="`xbc_get_val ${1}.name`
+	fi
+	for __var in `xbc_subkeys "${1}.var" 1`; do
+		echo -n ":$__var="`xbc_get_val ${1}.var.${__var}`
+	done
+	if xbc_has_key "${1}.pause"; then
+		echo -n ":pause"
+	elif xbc_has_key "${1}.continue"; then
+		echo -n ":continue"
+	elif xbc_has_key "${1}.clear"; then
+		echo -n ":clear"
+	fi
+	print_hist_actions ${1} "onmax" "var"
+	print_hist_actions ${1} "onchange" "var"
+	print_hist_actions ${1} "onmatch" "event"
+
+	if xbc_has_key "${1}.filter"; then
+		echo -n " if "`xbc_get_val ${1}.filter`
+	fi
+}
+
+setup_one_histogram() { # prefix trigger-file
+	run_cmd "echo '`print_one_histogram ${1}`' >> ${2}"
+}
+
+setup_histograms() { # prefix trigger-file
+	for __hist in `xbc_subkeys ${1} 1 ".[0-9]"`; do
+		setup_one_histogram ${1}.$__hist ${2}
+	done
+	if xbc_has_key ${1}.keys; then
+		setup_one_histogram ${1} ${2}
+	fi
+}
+
 setup_event() { # prefix group event [instance]
 	branch=$1.$2.$3
 	if [ "$4" ]; then
@@ -121,6 +203,8 @@ setup_event() { # prefix group event [instance]
 	set_value_of ${branch}.filter ${eventdir}/filter
 	set_array_of ${branch}.actions ${eventdir}/trigger
 
+	setup_histograms ${branch}.hist ${eventdir}/trigger
+
 	if xbc_has_key ${branch}.enable; then
 		run_cmd "echo 1 > ${eventdir}/enable"
 	fi
diff --git a/tools/bootconfig/scripts/xbc.sh b/tools/bootconfig/scripts/xbc.sh
index b8c84e654556..1f0ebf50dd2d 100644
--- a/tools/bootconfig/scripts/xbc.sh
+++ b/tools/bootconfig/scripts/xbc.sh
@@ -49,8 +49,8 @@ xbc_has_branch() { # prefix-key
 	grep -q "^$1" $XBC_TMPFILE
 }
 
-xbc_subkeys() { # prefix-key depth
+xbc_subkeys() { # prefix-key depth [subkey-pattern]
 	__keys=`echo $1 | sed "s/\./ /g"`
 	__s=`nr_args $__keys`
-	grep "^$1" $XBC_TMPFILE | cut -d= -f1| cut -d. -f$((__s + 1))-$((__s + $2)) | uniq
+	grep "^$1$3" $XBC_TMPFILE | cut -d= -f1| cut -d. -f$((__s + 1))-$((__s + $2)) | uniq
 }

[PATCH v2 9/9] tools/bootconfig: Use per-group/all enable option in ftrace2bconf script

[Masami Hiramatsu]

Use per-group/all enable option instead of ftrace.events option.
This will make the bootconfig file more readable.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/scripts/ftrace2bconf.sh |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/tools/bootconfig/scripts/ftrace2bconf.sh b/tools/bootconfig/scripts/ftrace2bconf.sh
index a0c3bcc6da4f..fbaf07dc91bf 100755
--- a/tools/bootconfig/scripts/ftrace2bconf.sh
+++ b/tools/bootconfig/scripts/ftrace2bconf.sh
@@ -92,6 +92,10 @@ referred_vars() {
 	grep "^hist" $1/trigger | grep -o '$[a-zA-Z0-9]*'
 }
 
+event_is_enabled() { # enable-file
+	test -f $1 & grep -q "1" $1
+}
+
 per_event_options() { # event-dir
 	evdir=$1
 	# Check the special event which has no filter and no trigger
@@ -113,7 +117,9 @@ per_event_options() { # event-dir
 		emit_kv $PREFIX.event.$group.$event.actions += \'$action\'
 	done
 
-	# enable is not checked; this is done by set_event in the instance.
+	if [ $GROUP_ENABLED -eq 0 ] && event_is_enabled $evdir/enable; then
+		emit_kv $PREFIX.event.$group.$event.enable
+	fi
 	val=`cat $evdir/filter`
 	if [ "$val" != "none" ]; then
 		emit_kv $PREFIX.event.$group.$event.filter = "$val"
@@ -137,8 +143,19 @@ event_options() {
 		kprobe_event_options
 		synth_event_options
 	fi
+	ALL_ENABLED=0
+	if event_is_enabled $INSTANCE/events/enable; then
+		emit_kv $PREFIX.event.enable
+		ALL_ENABLED=1
+	fi
 	for group in `ls $INSTANCE/events/` ; do
 		[ ! -d $INSTANCE/events/$group ] && continue
+		GROUP_ENABLED=$ALL_ENABLED
+		if [ $ALL_ENABLED -eq 0 ] && \
+		   event_is_enabled $INSTANCE/events/$group/enable ;then
+			emit_kv $PREFIX.event.$group.enable
+			GROUP_ENABLED=1
+		fi
 		for event in `ls $INSTANCE/events/$group/` ;do
 			[ ! -d $INSTANCE/events/$group/$event ] && continue
 			per_event_options $INSTANCE/events/$group/$event
@@ -226,11 +243,6 @@ instance_options() { # [instance-name]
 		emit_kv $PREFIX.tracing_on = $val
 	fi
 
-	val=
-	for i in `cat $INSTANCE/set_event`; do
-		val="$val, $i"
-	done
-	[ "$val" ] && emit_kv $PREFIX.events = "${val#,}"
 	val=`cat $INSTANCE/current_tracer`
 	[ $val != nop ] && emit_kv $PREFIX.tracer = $val
 	if grep -qv "^#" $INSTANCE/set_ftrace_filter $INSTANCE/set_ftrace_notrace; then

Re: [PATCH v2 5/9] tracing/boot: Show correct histogram error command

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5845 bytes --]

Hi Masami,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tip/perf/core]
[also build test WARNING on trace/for-next linux/master linus/master v5.14-rc4 next-20210805]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/tracing-boot-Add-histogram-syntax-support-in-boot-time-tracing/20210806-002938
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 79551ec0782895af27d6aa9b3abb6d547b7260d3
config: x86_64-randconfig-r025-20210805 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 42b9c2a17a0b63cccf3ac197a82f91b28e53e643)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b07ebb734f4e2b68934de501715c1cd98e34ae17
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Masami-Hiramatsu/tracing-boot-Add-histogram-syntax-support-in-boot-time-tracing/20210806-002938
        git checkout b07ebb734f4e2b68934de501715c1cd98e34ae17
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/trace/trace_boot.c:287:2: error: implicit declaration of function 'xbc_node_for_each_subkey' [-Werror,-Wimplicit-function-declaration]
           xbc_node_for_each_subkey(hnode, node) {
           ^
   kernel/trace/trace_boot.c:287:39: error: expected ';' after expression
           xbc_node_for_each_subkey(hnode, node) {
                                                ^
                                                ;
   kernel/trace/trace_boot.c:290:4: error: 'continue' statement not in loop statement
                           continue;
                           ^
   kernel/trace/trace_boot.c:294:4: error: 'break' statement not in loop or switch statement
                           break;
                           ^
   kernel/trace/trace_boot.c:404:2: error: implicit declaration of function 'xbc_node_for_each_subkey' [-Werror,-Wimplicit-function-declaration]
           xbc_node_for_each_subkey(hnode, node) {
           ^
   kernel/trace/trace_boot.c:404:39: error: expected ';' after expression
           xbc_node_for_each_subkey(hnode, node) {
                                                ^
                                                ;
   kernel/trace/trace_boot.c:407:4: error: 'continue' statement not in loop statement
                           continue;
                           ^
>> kernel/trace/trace_boot.c:420:4: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
                           if (trigger_process_regex(file, buf) < 0)
                           ^
   kernel/trace/trace_boot.c:418:3: note: previous statement is here
                   if (trace_boot_compose_hist_cmd(hnode, buf, size) == 0)
                   ^
   1 warning and 7 errors generated.


vim +/if +420 kernel/trace/trace_boot.c

8885ed81dfd529 Masami Hiramatsu 2021-08-06  395  
8885ed81dfd529 Masami Hiramatsu 2021-08-06  396  static void __init
8885ed81dfd529 Masami Hiramatsu 2021-08-06  397  trace_boot_init_histograms(struct trace_event_file *file,
8885ed81dfd529 Masami Hiramatsu 2021-08-06  398  			   struct xbc_node *hnode, char *buf, size_t size)
8885ed81dfd529 Masami Hiramatsu 2021-08-06  399  {
8885ed81dfd529 Masami Hiramatsu 2021-08-06  400  	struct xbc_node *node;
8885ed81dfd529 Masami Hiramatsu 2021-08-06  401  	const char *p;
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  402  	char *tmp;
8885ed81dfd529 Masami Hiramatsu 2021-08-06  403  
8885ed81dfd529 Masami Hiramatsu 2021-08-06 @404  	xbc_node_for_each_subkey(hnode, node) {
8885ed81dfd529 Masami Hiramatsu 2021-08-06  405  		p = xbc_node_get_data(node);
8885ed81dfd529 Masami Hiramatsu 2021-08-06  406  		if (!isdigit(p[0]))
8885ed81dfd529 Masami Hiramatsu 2021-08-06  407  			continue;
8885ed81dfd529 Masami Hiramatsu 2021-08-06  408  		/* All digit started node should be instances. */
8885ed81dfd529 Masami Hiramatsu 2021-08-06  409  		if (trace_boot_compose_hist_cmd(node, buf, size) == 0) {
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  410  			tmp = kstrdup(buf, GFP_KERNEL);
8885ed81dfd529 Masami Hiramatsu 2021-08-06  411  			if (trigger_process_regex(file, buf) < 0)
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  412  				pr_err("Failed to apply hist trigger: %s\n", tmp);
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  413  			kfree(tmp);
8885ed81dfd529 Masami Hiramatsu 2021-08-06  414  		}
8885ed81dfd529 Masami Hiramatsu 2021-08-06  415  	}
8885ed81dfd529 Masami Hiramatsu 2021-08-06  416  
8885ed81dfd529 Masami Hiramatsu 2021-08-06  417  	if (xbc_node_find_child(hnode, "keys")) {
8885ed81dfd529 Masami Hiramatsu 2021-08-06  418  		if (trace_boot_compose_hist_cmd(hnode, buf, size) == 0)
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  419  			tmp = kstrdup(buf, GFP_KERNEL);
8885ed81dfd529 Masami Hiramatsu 2021-08-06 @420  			if (trigger_process_regex(file, buf) < 0)
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  421  				pr_err("Failed to apply hist trigger: %s\n", tmp);
b07ebb734f4e2b Masami Hiramatsu 2021-08-06  422  			kfree(tmp);
8885ed81dfd529 Masami Hiramatsu 2021-08-06  423  	}
8885ed81dfd529 Masami Hiramatsu 2021-08-06  424  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29512 bytes --]

Re: [PATCH v2 5/9] tracing/boot: Show correct histogram error command

[Masami Hiramatsu]

On Fri, 6 Aug 2021 10:58:30 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Masami,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on tip/perf/core]
> [also build test WARNING on trace/for-next linux/master linus/master v5.14-rc4 next-20210805]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/tracing-boot-Add-histogram-syntax-support-in-boot-time-tracing/20210806-002938
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 79551ec0782895af27d6aa9b3abb6d547b7260d3

This seems that the base tree is not correct. I worked on Steve's 
linux-trace tree for this series, including 

commit e5efaeb8a8f5 ("bootconfig: Support mixing a value and subkeys under a key")

which introduced "xbc_node_for_each_subkey()" macro.

Thank you,


> config: x86_64-randconfig-r025-20210805 (attached as .config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 42b9c2a17a0b63cccf3ac197a82f91b28e53e643)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/b07ebb734f4e2b68934de501715c1cd98e34ae17
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Masami-Hiramatsu/tracing-boot-Add-histogram-syntax-support-in-boot-time-tracing/20210806-002938
>         git checkout b07ebb734f4e2b68934de501715c1cd98e34ae17
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>    kernel/trace/trace_boot.c:287:2: error: implicit declaration of function 'xbc_node_for_each_subkey' [-Werror,-Wimplicit-function-declaration]
>            xbc_node_for_each_subkey(hnode, node) {
>            ^
>    kernel/trace/trace_boot.c:287:39: error: expected ';' after expression
>            xbc_node_for_each_subkey(hnode, node) {
>                                                 ^
>                                                 ;
>    kernel/trace/trace_boot.c:290:4: error: 'continue' statement not in loop statement
>                            continue;
>                            ^
>    kernel/trace/trace_boot.c:294:4: error: 'break' statement not in loop or switch statement
>                            break;
>                            ^
>    kernel/trace/trace_boot.c:404:2: error: implicit declaration of function 'xbc_node_for_each_subkey' [-Werror,-Wimplicit-function-declaration]
>            xbc_node_for_each_subkey(hnode, node) {
>            ^
>    kernel/trace/trace_boot.c:404:39: error: expected ';' after expression
>            xbc_node_for_each_subkey(hnode, node) {
>                                                 ^
>                                                 ;
>    kernel/trace/trace_boot.c:407:4: error: 'continue' statement not in loop statement
>                            continue;
>                            ^
> >> kernel/trace/trace_boot.c:420:4: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]
>                            if (trigger_process_regex(file, buf) < 0)
>                            ^
>    kernel/trace/trace_boot.c:418:3: note: previous statement is here
>                    if (trace_boot_compose_hist_cmd(hnode, buf, size) == 0)
>                    ^
>    1 warning and 7 errors generated.
> 
> 
> vim +/if +420 kernel/trace/trace_boot.c
> 
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  395  
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  396  static void __init
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  397  trace_boot_init_histograms(struct trace_event_file *file,
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  398  			   struct xbc_node *hnode, char *buf, size_t size)
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  399  {
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  400  	struct xbc_node *node;
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  401  	const char *p;
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  402  	char *tmp;
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  403  
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06 @404  	xbc_node_for_each_subkey(hnode, node) {
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  405  		p = xbc_node_get_data(node);
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  406  		if (!isdigit(p[0]))
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  407  			continue;
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  408  		/* All digit started node should be instances. */
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  409  		if (trace_boot_compose_hist_cmd(node, buf, size) == 0) {
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  410  			tmp = kstrdup(buf, GFP_KERNEL);
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  411  			if (trigger_process_regex(file, buf) < 0)
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  412  				pr_err("Failed to apply hist trigger: %s\n", tmp);
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  413  			kfree(tmp);
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  414  		}
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  415  	}
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  416  
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  417  	if (xbc_node_find_child(hnode, "keys")) {
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  418  		if (trace_boot_compose_hist_cmd(hnode, buf, size) == 0)
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  419  			tmp = kstrdup(buf, GFP_KERNEL);
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06 @420  			if (trigger_process_regex(file, buf) < 0)
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  421  				pr_err("Failed to apply hist trigger: %s\n", tmp);
> b07ebb734f4e2b Masami Hiramatsu 2021-08-06  422  			kfree(tmp);
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  423  	}
> 8885ed81dfd529 Masami Hiramatsu 2021-08-06  424  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 5/9] tracing/boot: Show correct histogram error command

[Steven Rostedt]

On Fri, 6 Aug 2021 23:00:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> This seems that the base tree is not correct. I worked on Steve's 
> linux-trace tree for this series, including 
> 
> commit e5efaeb8a8f5 ("bootconfig: Support mixing a value and subkeys under a key")
> 
> which introduced "xbc_node_for_each_subkey()" macro.

Thanks for confirming. I figured as much.

I'll be pushing more 5.15 changes once my final 5.14 fixes land in
Linus's tree, and I can rebase on top of rc5.

-- Steve

Re: [PATCH v2 1/9] tracing/boot: Fix a hist trigger dependency for boot time tracing

[Steven Rostedt]

On Fri,  6 Aug 2021 01:25:29 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fixes a build error when CONFIG_HIST_TRIGGERS=n with boot-time
> tracing. Since the trigger_process_regex() is defined only
> when CONFIG_HIST_TRIGGERS=y, if it is disabled, the 'actions'
> event option also must be disabled.
> 
> Fixes: 81a59555ff15 ("tracing/boot: Add per-event settings")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  0 files changed
> 
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index 94ef2d099e32..e6dc9269ad75 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -204,13 +204,14 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
>  		else if (apply_event_filter(file, buf) < 0)
>  			pr_err("Failed to apply filter: %s\n", buf);
>  	}
> -
> +#ifdef CONFIG_HIST_TRIGGERS

Hi Masamai,

Can we instead define trigger_process_regex() in trace.h to be:

static inline int trigger_process_regex(struct trace_event_file *file, char *buff)
{
	return -1;
}

When this config is not set?

This makes the code a bit cleaner, and you get the "Failed to apply an
action" error as well.

-- Steve



>  	xbc_node_for_each_array_value(enode, "actions", anode, p) {
>  		if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
>  			pr_err("action string is too long: %s\n", p);
>  		else if (trigger_process_regex(file, buf) < 0)
>  			pr_err("Failed to apply an action: %s\n", buf);
>  	}
> +#endif
>  
>  	if (xbc_node_find_value(enode, "enable", NULL)) {
>  		if (trace_event_enable_disable(file, 1, 0) < 0)

Re: [PATCH v2 1/9] tracing/boot: Fix a hist trigger dependency for boot time tracing

[Masami Hiramatsu]

On Fri, 6 Aug 2021 21:47:10 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri,  6 Aug 2021 01:25:29 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Fixes a build error when CONFIG_HIST_TRIGGERS=n with boot-time
> > tracing. Since the trigger_process_regex() is defined only
> > when CONFIG_HIST_TRIGGERS=y, if it is disabled, the 'actions'
> > event option also must be disabled.
> > 
> > Fixes: 81a59555ff15 ("tracing/boot: Add per-event settings")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  0 files changed
> > 
> > diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> > index 94ef2d099e32..e6dc9269ad75 100644
> > --- a/kernel/trace/trace_boot.c
> > +++ b/kernel/trace/trace_boot.c
> > @@ -204,13 +204,14 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode,
> >  		else if (apply_event_filter(file, buf) < 0)
> >  			pr_err("Failed to apply filter: %s\n", buf);
> >  	}
> > -
> > +#ifdef CONFIG_HIST_TRIGGERS
> 
> Hi Masamai,
> 
> Can we instead define trigger_process_regex() in trace.h to be:
> 
> static inline int trigger_process_regex(struct trace_event_file *file, char *buff)
> {
> 	return -1;
> }
> 
> When this config is not set?
> 
> This makes the code a bit cleaner, and you get the "Failed to apply an
> action" error as well.

Ah, OK.
But it seems that the error message should be changed a bit in that
case, because the actions string is correct, but the kernel
just doesn't support it. Moreover, it loops on the array, so, it will
show same error repeatedly.

I think it should be ignored, or just warn once with correct reason.
(for example, if the kernel supports hist-trigger, the error will
be shown in 'error_log' file.)

Thank you,


> 
> -- Steve
> 
> 
> 
> >  	xbc_node_for_each_array_value(enode, "actions", anode, p) {
> >  		if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf))
> >  			pr_err("action string is too long: %s\n", p);
> >  		else if (trigger_process_regex(file, buf) < 0)
> >  			pr_err("Failed to apply an action: %s\n", buf);
> >  	}
> > +#endif
> >  
> >  	if (xbc_node_find_value(enode, "enable", NULL)) {
> >  		if (trace_event_enable_disable(file, 1, 0) < 0)
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>