Skip to contentSkip to conversation

tracing: Really, this is the last update for 5.6 before my pull request!

PATCH In review Steven Rostedt 2 participants 7 messages Feb 5, 2020

Subsystems (5)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover tracing: Really, this is the last update for 5.6 before my pull request!

Masami Hiramatsu (4):
bootconfig: Use bootconfig instead of boot config
bootconfig: Add more parse error messages tools/bootconfig: Show the number of bootconfig nodes
bootconfig: Show the number of nodes on boot message

Steven Rostedt (VMware) (1):
ftrace: Protect ftrace_graph_hash with ftrace_sync

----

 init/main.c             | 10 ++++++----
 kernel/trace/ftrace.c   | 11 +++++++++--
 kernel/trace/trace.h    |  2 ++
 lib/bootconfig.c        | 21 ++++++++++++++++-----
 tools/bootconfig/main.c |  1 +
 5 files changed, 34 insertions(+), 11 deletions(-)
author maintainer
#1 lore raw

PATCH 1/5 ftrace: Protect ftrace_graph_hash with ftrace_sync

From: "Steven Rostedt (VMware)" <[email protected]>

As function_graph tracer can run when RCU is not "watching", it can not be protected by synchronize_rcu() it requires running a task on each CPU before it can be freed. Calling schedule_on_each_cpu(ftrace_sync) needs to be used.

Fixes b9b0c831bed26 ("ftrace: Convert graph filter to use hash tables")
---
 kernel/trace/ftrace.c | 11 +++++++++--
 kernel/trace/trace.h  |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)
kernel/trace/ftrace.c +9 -2
index 481ede3eac13..3f7ee102868a 100644
@@ -5867,8 +5867,15 @@ ftrace_graph_release(struct inode *inode, struct file *file)
 
 		mutex_unlock(&graph_lock);
 
-		/* Wait till all users are no longer using the old hash */
-		synchronize_rcu();
+		/*
+		 * We need to do a hard force of sched synchronization.
+		 * This is because we use preempt_disable() to do RCU, but
+		 * the function tracers can be called where RCU is not watching
+		 * (like before user_exit()). We can not rely on the RCU
+		 * infrastructure to do the synchronization, thus we must do it
+		 * ourselves.
+		 */
+		schedule_on_each_cpu(ftrace_sync);
 
 		free_ftrace_hash(old_hash);
 	}
kernel/trace/trace.h +2 -0
index 8c52f5de9384..3c75d29bd861 100644
@@ -979,6 +979,7 @@ static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
 	 * Have to open code "rcu_dereference_sched()" because the
 	 * function graph tracer can be called when RCU is not
 	 * "watching".
+	 * Protected with schedule_on_each_cpu(ftrace_sync)
 	 */
 	hash = rcu_dereference_protected(ftrace_graph_hash, !preemptible());
 
@@ -1031,6 +1032,7 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr)
 	 * Have to open code "rcu_dereference_sched()" because the
 	 * function graph tracer can be called when RCU is not
 	 * "watching".
+	 * Protected with schedule_on_each_cpu(ftrace_sync)
 	 */
 	notrace_hash = rcu_dereference_protected(ftrace_graph_notrace_hash,
 						 !preemptible());
Signature · 1 line
2.24.1
Reviewed-by on v1 1/5
#2 lore raw

Re: [for-next][PATCH 1/5] ftrace: Protect ftrace_graph_hash with ftrace_sync

Quoted 6 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

As function_graph tracer can run when RCU is not "watching", it can not be protected by synchronize_rcu() it requires running a task on each CPU before it can be freed. Calling schedule_on_each_cpu(ftrace_sync) needs to be used.

Quoted 7 lines
Fixes b9b0c831bed26 ("ftrace: Convert graph filter to use hash tables")

Nice! If there is much more call for this, perhaps I should take a hint from the ftrace_sync() comment and add synchronize_rcu_rude(). ;-)

Quoted 51 lines
---
 kernel/trace/ftrace.c | 11 +++++++++--
 kernel/trace/trace.h  |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)
kernel/trace/ftrace.c +9 -2
index 481ede3eac13..3f7ee102868a 100644
@@ -5867,8 +5867,15 @@ ftrace_graph_release(struct inode *inode, struct file *file)
 
 		mutex_unlock(&graph_lock);
 
-		/* Wait till all users are no longer using the old hash */
-		synchronize_rcu();
+		/*
+		 * We need to do a hard force of sched synchronization.
+		 * This is because we use preempt_disable() to do RCU, but
+		 * the function tracers can be called where RCU is not watching
+		 * (like before user_exit()). We can not rely on the RCU
+		 * infrastructure to do the synchronization, thus we must do it
+		 * ourselves.
+		 */
+		schedule_on_each_cpu(ftrace_sync);
 
 		free_ftrace_hash(old_hash);
 	}
kernel/trace/trace.h +2 -0
index 8c52f5de9384..3c75d29bd861 100644
@@ -979,6 +979,7 @@ static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
 	 * Have to open code "rcu_dereference_sched()" because the
 	 * function graph tracer can be called when RCU is not
 	 * "watching".
+	 * Protected with schedule_on_each_cpu(ftrace_sync)
 	 */
 	hash = rcu_dereference_protected(ftrace_graph_hash, !preemptible());
 
@@ -1031,6 +1032,7 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr)
 	 * Have to open code "rcu_dereference_sched()" because the
 	 * function graph tracer can be called when RCU is not
 	 * "watching".
+	 * Protected with schedule_on_each_cpu(ftrace_sync)
 	 */
 	notrace_hash = rcu_dereference_protected(ftrace_graph_notrace_hash,
 						 !preemptible());
Signature · 1 line
2.24.1
author maintainer
#3 lore raw

PATCH 2/5 bootconfig: Use bootconfig instead of boot config

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Use "bootconfig" (1 word) instead of "boot config" (2 words) in the boot message.

---
 init/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
init/main.c +3 -3
index f174a59d3903..2de2f9f7aab9 100644
@@ -372,7 +372,7 @@ static void __init setup_boot_config(const char *cmdline)
 
 	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
 	if (!copy) {
-		pr_err("Failed to allocate memory for boot config\n");
+		pr_err("Failed to allocate memory for bootconfig\n");
 		return;
 	}
 
@@ -380,9 +380,9 @@ static void __init setup_boot_config(const char *cmdline)
 	copy[size] = '\0';
 
 	if (xbc_init(copy) < 0)
-		pr_err("Failed to parse boot config\n");
+		pr_err("Failed to parse bootconfig\n");
 	else {
-		pr_info("Load boot config: %d bytes\n", size);
+		pr_info("Load bootconfig: %d bytes\n", size);
 		/* keys starting with "kernel." are passed via cmdline */
 		extra_command_line = xbc_make_cmdline("kernel");
 		/* Also, "init." keys are init arguments */
Signature · 1 line
2.24.1
author maintainer
#4 lore raw

PATCH 3/5 bootconfig: Add more parse error messages

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Add more error messages for following cases.
- Exceeding max number of nodes
- Config tree data is empty (e.g. comment only)
- Config data is empty or exceeding max size
- bootconfig is already initialized

---
 lib/bootconfig.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
lib/bootconfig.c +12 -4
index 055014e233a5..a98ae136529c 100644
@@ -373,7 +373,8 @@ static struct xbc_node * __init xbc_add_sibling(char *data, u32 flag)
 				sib->next = xbc_node_index(node);
 			}
 		}
-	}
+	} else
+		xbc_parse_error("Too many nodes", data);
 
 	return node;
 }
@@ -657,8 +658,10 @@ static int __init xbc_verify_tree(void)
 	struct xbc_node *n, *m;
 
 	/* Empty tree */
-	if (xbc_node_num == 0)
+	if (xbc_node_num == 0) {
+		xbc_parse_error("Empty config", xbc_data);
 		return -ENOENT;
+	}
 
 	for (i = 0; i < xbc_node_num; i++) {
 		if (xbc_nodes[i].next > xbc_node_num) {
@@ -732,12 +735,17 @@ int __init xbc_init(char *buf)
 	char *p, *q;
 	int ret, c;
 
-	if (xbc_data)
+	if (xbc_data) {
+		pr_err("Error: bootconfig is already initialized.\n");
 		return -EBUSY;
+	}
 
 	ret = strlen(buf);
-	if (ret > XBC_DATA_MAX - 1 || ret == 0)
+	if (ret > XBC_DATA_MAX - 1 || ret == 0) {
+		pr_err("Error: Config data is %s.\n",
+			ret ? "too big" : "empty");
 		return -ERANGE;
+	}
 
 	xbc_data = buf;
 	xbc_data_size = ret + 1;
Signature · 1 line
2.24.1
author maintainer
#5 lore raw

PATCH 4/5 tools/bootconfig: Show the number of bootconfig nodes

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Show the number of bootconfig nodes when applying new bootconfig to initrd.

Since there are limitations of bootconfig not only in its filesize, but also the number of nodes, the number should be shown when applying so that user can get the feeling of scale of current bootconfig.

---
 lib/bootconfig.c        | 5 ++++-
 tools/bootconfig/main.c | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
lib/bootconfig.c +4 -1
index a98ae136529c..afb2e767e6fe 100644
@@ -728,7 +728,8 @@ void __init xbc_destroy_all(void)
  *
  * This parses the boot config text in @buf. @buf must be a
  * null terminated string and smaller than XBC_DATA_MAX.
- * Return 0 if succeeded, or -errno if there is any error.
+ * Return the number of stored nodes (>0) if succeeded, or -errno
+ * if there is any error.
  */
 int __init xbc_init(char *buf)
 {
@@ -788,6 +789,8 @@ int __init xbc_init(char *buf)
 
 	if (ret < 0)
 		xbc_destroy_all();
+	else
+		ret = xbc_node_num;
 
 	return ret;
 }
tools/bootconfig/main.c +1 -0
index 91c9a5c0c499..47f488458328 100644
@@ -268,6 +268,7 @@ int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	printf("Apply %s to %s\n", xbc_path, path);
+	printf("\tNumber of nodes: %d\n", ret);
 	printf("\tSize: %u bytes\n", (unsigned int)size);
 	printf("\tChecksum: %d\n", (unsigned int)csum);
 
Signature · 1 line
2.24.1
author maintainer
#6 lore raw

PATCH 5/5 bootconfig: Show the number of nodes on boot message

In reply to Steven Rostedt

From: Masami Hiramatsu <[email protected]>

Show the number of bootconfig nodes on boot message.

---
 init/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
init/main.c +4 -2
index 2de2f9f7aab9..491f1cdb3105 100644
@@ -342,6 +342,7 @@ static void __init setup_boot_config(const char *cmdline)
 	char *data, *copy;
 	const char *p;
 	u32 *hdr;
+	int ret;
 
 	p = strstr(cmdline, "bootconfig");
 	if (!p || (p != cmdline && !isspace(*(p-1))) ||
@@ -379,10 +380,11 @@ static void __init setup_boot_config(const char *cmdline)
 	memcpy(copy, data, size);
 	copy[size] = '\0';
 
-	if (xbc_init(copy) < 0)
+	ret = xbc_init(copy);
+	if (ret < 0)
 		pr_err("Failed to parse bootconfig\n");
 	else {
-		pr_info("Load bootconfig: %d bytes\n", size);
+		pr_info("Load bootconfig: %d bytes %d nodes\n", size, ret);
 		/* keys starting with "kernel." are passed via cmdline */
 		extra_command_line = xbc_make_cmdline("kernel");
 		/* Also, "init." keys are init arguments */
Signature · 1 line
2.24.1