ftrace: Allow to change size of function graph filters

RFC/PATCH Namhyung Kim 2 participants 4 messages Jan 13, 2017 – Jan 14, 2017

Subsystems (2)

Tracing ftrace

lore

Share & tools

lore

Namhyung Kim

author

2017-01-13 04:25 UTC #0 lore raw

It's currently fixed to 32 and it ignores when user gives a pattern which match to functions more than the size. So filtering like all system calls or many functions with common prefix cannot be set all. Not sure this is right though.

This patch adds 'graph_filter_size' file in the tracefs to adjust the size. It can be changed only if the current tracer is not set.

Signed-off-by Namhyung Kim <[email protected]>

---
 kernel/trace/ftrace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/trace/trace.c  |   8 +++
 kernel/trace/trace.h  |   9 ++-
 3 files changed, 157 insertions(+), 11 deletions(-)

kernel/trace/ftrace.c +143 -8

index 356bb70d071e..f8440b2bf546 100644
@@ -4525,8 +4525,11 @@ static DEFINE_MUTEX(graph_lock);
 
 int ftrace_graph_count;
 int ftrace_graph_notrace_count;
-unsigned long ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS] __read_mostly;
-unsigned long ftrace_graph_notrace_funcs[FTRACE_GRAPH_MAX_FUNCS] __read_mostly;
+int ftrace_graph_funcs_size = FTRACE_GRAPH_MAX_FUNCS;
+unsigned long __ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS] __read_mostly;
+unsigned long __ftrace_graph_notrace_funcs[FTRACE_GRAPH_MAX_FUNCS] __read_mostly;
+unsigned long *ftrace_graph_funcs = __ftrace_graph_funcs;
+unsigned long *ftrace_graph_notrace_funcs = __ftrace_graph_notrace_funcs;
 
 struct ftrace_graph_data {
 	unsigned long *table;

@@ -4558,6 +4561,12 @@ static void *g_start(struct seq_file *m, loff_t *pos)
 
 	mutex_lock(&graph_lock);
 
+	/* it might be changed, reload the pointers */
+	if (fgd->count == &ftrace_graph_count)
+		fgd->table = ftrace_graph_funcs;
+	else
+		fgd->table = ftrace_graph_notrace_funcs;
+
 	/* Nothing, tell g_show to print all functions are enabled */
 	if (!*fgd->count && !*pos)
 		return (void *)1;

@@ -4605,13 +4614,11 @@ __ftrace_graph_open(struct inode *inode, struct file *file,
 {
 	int ret = 0;
 
-	mutex_lock(&graph_lock);
 	if ((file->f_mode & FMODE_WRITE) &&
 	    (file->f_flags & O_TRUNC)) {
 		*fgd->count = 0;
 		memset(fgd->table, 0, fgd->size * sizeof(*fgd->table));
 	}
-	mutex_unlock(&graph_lock);
 
 	if (file->f_mode & FMODE_READ) {
 		ret = seq_open(file, fgd->seq_ops);

@@ -4629,6 +4636,7 @@ static int
 ftrace_graph_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_graph_data *fgd;
+	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;

@@ -4637,18 +4645,26 @@ ftrace_graph_open(struct inode *inode, struct file *file)
 	if (fgd == NULL)
 		return -ENOMEM;
 
+	mutex_lock(&graph_lock);
+
 	fgd->table = ftrace_graph_funcs;
-	fgd->size = FTRACE_GRAPH_MAX_FUNCS;
+	fgd->size = ftrace_graph_funcs_size;
 	fgd->count = &ftrace_graph_count;
 	fgd->seq_ops = &ftrace_graph_seq_ops;
 
-	return __ftrace_graph_open(inode, file, fgd);
+	ret = __ftrace_graph_open(inode, file, fgd);
+	if (ret < 0)
+		kfree(fgd);
+
+	mutex_unlock(&graph_lock);
+	return ret;
 }
 
 static int
 ftrace_graph_notrace_open(struct inode *inode, struct file *file)
 {
 	struct ftrace_graph_data *fgd;
+	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return -ENODEV;

@@ -4657,12 +4673,19 @@ ftrace_graph_notrace_open(struct inode *inode, struct file *file)
 	if (fgd == NULL)
 		return -ENOMEM;
 
+	mutex_lock(&graph_lock);
+
 	fgd->table = ftrace_graph_notrace_funcs;
-	fgd->size = FTRACE_GRAPH_MAX_FUNCS;
+	fgd->size = ftrace_graph_funcs_size;
 	fgd->count = &ftrace_graph_notrace_count;
 	fgd->seq_ops = &ftrace_graph_seq_ops;
 
-	return __ftrace_graph_open(inode, file, fgd);
+	ret = __ftrace_graph_open(inode, file, fgd);
+	if (ret < 0)
+		kfree(fgd);
+
+	mutex_unlock(&graph_lock);
+	return ret;
 }
 
 static int

@@ -4767,6 +4790,13 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,
 
 		mutex_lock(&graph_lock);
 
+		/* it might be changed, reload the pointers */
+		if (fgd->count == &ftrace_graph_count)
+			fgd->table = ftrace_graph_funcs;
+		else
+			fgd->table = ftrace_graph_notrace_funcs;
+		fgd->size = ftrace_graph_funcs_size;
+
 		/* we allow only one expression at a time */
 		ret = ftrace_set_func(fgd->table, fgd->count, fgd->size,
 				      parser.buffer);

@@ -4782,6 +4812,102 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,
 	return ret;
 }
 
+static ssize_t
+ftrace_graph_funcs_size_read(struct file *filp, char __user *ubuf,
+			     size_t cnt, loff_t *ppos)
+{
+	char buf[64];		/* big enough to hold a number */
+	int r;
+
+	r = sprintf(buf, "%d\n", ftrace_graph_funcs_size);
+	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
+}
+
+static ssize_t
+ftrace_graph_funcs_size_write(struct file *file, const char __user *ubuf,
+			      size_t cnt, loff_t *ppos)
+{
+	unsigned long *pfuncs, *old_funcs;
+	unsigned long *pnotrace, *old_notrace;
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val == 0 || val > INT_MAX)
+		return -EINVAL;
+
+	mutex_lock(&graph_lock);
+
+	if (val == ftrace_graph_funcs_size)
+		goto out;
+
+	if (val < max(ftrace_graph_count, ftrace_graph_notrace_count)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	mutex_lock(&trace_types_lock);
+
+	/* ensure function graph tracer is not running */
+	if (!tracing_is_nop()) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	old_funcs = ftrace_graph_funcs;
+	old_notrace = ftrace_graph_notrace_funcs;
+
+	if (val <= FTRACE_GRAPH_MAX_FUNCS) {
+		if (old_funcs == __ftrace_graph_funcs) {
+			ftrace_graph_funcs_size = val;
+			goto out_unlock;
+		}
+
+		pfuncs   = __ftrace_graph_funcs;
+		pnotrace = __ftrace_graph_notrace_funcs;
+	} else {
+		pfuncs = kmalloc_array(val, sizeof(long), GFP_KERNEL);
+		if (!pfuncs) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+
+		pnotrace = kmalloc_array(val, sizeof(long), GFP_KERNEL);
+		if (!pnotrace) {
+			kfree(pfuncs);
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+	}
+
+	memcpy(pfuncs, old_funcs, ftrace_graph_count * sizeof(long));
+	memcpy(pnotrace, old_notrace, ftrace_graph_notrace_count * sizeof(long));
+
+	ftrace_graph_funcs = pfuncs;
+	ftrace_graph_notrace_funcs = pnotrace;
+	ftrace_graph_funcs_size = val;
+
+	if (old_funcs != __ftrace_graph_funcs)
+		kfree(old_funcs);
+	if (old_notrace != __ftrace_graph_notrace_funcs)
+		kfree(old_notrace);
+
+out_unlock:
+	mutex_unlock(&trace_types_lock);
+
+out:
+	if (ret == 0) {
+		*ppos += cnt;
+		ret = cnt;
+	}
+
+	mutex_unlock(&graph_lock);
+	return ret;
+}
+
 static const struct file_operations ftrace_graph_fops = {
 	.open		= ftrace_graph_open,
 	.read		= seq_read,

@@ -4797,6 +4923,13 @@ static const struct file_operations ftrace_graph_notrace_fops = {
 	.llseek		= tracing_lseek,
 	.release	= ftrace_graph_release,
 };
+
+static const struct file_operations ftrace_graph_filter_size_fops = {
+	.open		= tracing_open_generic,
+	.read		= ftrace_graph_funcs_size_read,
+	.write		= ftrace_graph_funcs_size_write,
+	.llseek		= tracing_lseek,
+};
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
 void ftrace_create_filter_files(struct ftrace_ops *ops,

@@ -4847,6 +4980,8 @@ static __init int ftrace_init_dyn_tracefs(struct dentry *d_tracer)
 	trace_create_file("set_graph_notrace", 0444, d_tracer,
 				    NULL,
 				    &ftrace_graph_notrace_fops);
+	trace_create_file("graph_filter_size", 0644, d_tracer,
+			  NULL, &ftrace_graph_filter_size_fops);
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
 	return 0;

kernel/trace/trace.c +8 -0

index 970aafe80b49..d25243ff346b 100644
@@ -1106,6 +1106,14 @@ int tracing_is_on(void)
 }
 EXPORT_SYMBOL_GPL(tracing_is_on);
 
+/**
+ * tracing_is_nop - show current tracer is not selected
+ */
+bool tracing_is_nop(void)
+{
+	return global_trace.current_trace == &nop_trace;
+}
+
 static int __init set_buf_size(char *str)
 {
 	unsigned long buf_size;

kernel/trace/trace.h +6 -3

index c2234494f40c..7d0d16125fdb 100644
@@ -579,6 +579,7 @@ void tracing_reset_all_online_cpus(void);
 int tracing_open_generic(struct inode *inode, struct file *filp);
 bool tracing_is_disabled(void);
 int tracer_tracing_is_on(struct trace_array *tr);
+bool tracing_is_nop(void);
 struct dentry *trace_create_file(const char *name,
 				 umode_t mode,
 				 struct dentry *parent,

@@ -789,12 +790,14 @@ extern void __trace_graph_return(struct trace_array *tr,
 
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-/* TODO: make this variable */
 #define FTRACE_GRAPH_MAX_FUNCS		32
+extern int ftrace_graph_funcs_size;
 extern int ftrace_graph_count;
-extern unsigned long ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS];
+extern unsigned long __ftrace_graph_funcs[FTRACE_GRAPH_MAX_FUNCS];
+extern unsigned long *ftrace_graph_funcs;
 extern int ftrace_graph_notrace_count;
-extern unsigned long ftrace_graph_notrace_funcs[FTRACE_GRAPH_MAX_FUNCS];
+extern unsigned long __ftrace_graph_notrace_funcs[FTRACE_GRAPH_MAX_FUNCS];
+extern unsigned long *ftrace_graph_notrace_funcs;
 
 static inline int ftrace_graph_addr(unsigned long addr)
 {

Signature · 1 line

2.11.0

Steven Rostedt

maintainer

2017-01-13 16:16 UTC #1 lore raw

Quoted 4 lines · On Fri, 13 Jan 2017 13:22:43 +0900 Namhyung Kim <[email protected]> wrote:

It's currently fixed to 32 and it ignores when user gives a pattern which match to functions more than the size. So filtering like all system calls or many functions with common prefix cannot be set all. Not sure this is right though.

Yes it's small, and there's a reason for it. So I'm giving a conditional nack to the patch.

Quoted 5 lines

This patch adds 'graph_filter_size' file in the tracefs to adjust the size. It can be changed only if the current tracer is not set.
Signed-off-by Namhyung Kim <[email protected]>

The condition is, we need to fix ftrace_graph_addr() first.

	for (i = 0; i < ftrace_graph_count; i++) {

That gets called at every function being traced. See where I'm heading with that? ;-)

We need to create a hash table or binary search first and make that function handle a large ftrace_graph_count before implementing your patch. Remove the linear search, replace it with either a binary search or a hash. But an O(n) algorithm at every function call is out of the question.

Thanks!

-- Steve

Namhyung Kim

author

2017-01-14 04:30 UTC #2 lore raw

Hi Steve,

Quoted 20 lines · Steven Rostedt

Quoted 4 lines · On Fri, 13 Jan 2017 13:22:43 +0900 Namhyung Kim <[email protected]> wrote:
It's currently fixed to 32 and it ignores when user gives a pattern which match to functions more than the size. So filtering like all system calls or many functions with common prefix cannot be set all. Not sure this is right though.
Yes it's small, and there's a reason for it. So I'm giving a conditional nack to the patch.
Quoted 5 lines
This patch adds 'graph_filter_size' file in the tracefs to adjust the size. It can be changed only if the current tracer is not set.
Signed-off-by Namhyung Kim <[email protected]>
The condition is, we need to fix ftrace_graph_addr() first.
	for (i = 0; i < ftrace_graph_count; i++) {

Quoted 8 lines

That gets called at every function being traced. See where I'm heading with that? ;-)
We need to create a hash table or binary search first and make that function handle a large ftrace_graph_count before implementing your patch. Remove the linear search, replace it with either a binary search or a hash. But an O(n) algorithm at every function call is out of the question.

Fair enough. I'll try to add a hash table then.

But I'm not sure how to synchronize hash table manipulations. It seems synchronize_sched() is not good enough for function graph tracer, right? So I limited changing filter size only when no tracer is used, but is it ok to have the limitation when adding or removing an entry to/from the table? If not, what can I do?

Thanks,
Namhyung

Steven Rostedt

maintainer

2017-01-14 04:50 UTC #3 lore raw

Quoted 5 lines · On Sat, 14 Jan 2017 13:20:00 +0900 Namhyung Kim <[email protected]> wrote:

But I'm not sure how to synchronize hash table manipulations. It seems synchronize_sched() is not good enough for function graph tracer, right? So I limited changing filter size only when no tracer is used, but is it ok to have the limitation when adding or removing an entry to/from the table? If not, what can I do?

There's already a hash that function tracing uses. Look at the code in ftrace_hash_move(). If you make it where the hash is always looked at with preemption disabled, you can use synchronize_sched() to modify the hash.

The trampolines are a bit trickier, because there's no way to disable preemption or do any other synchronization when they are called.

-- Steve