[RFC/PATCH] ftrace: Allow to change size of function graph filters

[RFC/PATCH] ftrace: Allow to change size of function graph filters

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 kernel/trace/ftrace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/trace/trace.c  |   8 +++
 kernel/trace/trace.h  |   9 ++-
 3 files changed, 157 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 356bb70d071e..f8440b2bf546 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 970aafe80b49..d25243ff346b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -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;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index c2234494f40c..7d0d16125fdb 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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)
 {
-- 
2.11.0

Re: [RFC/PATCH] ftrace: Allow to change size of function graph filters

[Steven Rostedt]

On Fri, 13 Jan 2017 13:22:43 +0900
Namhyung Kim <namhyung@kernel.org> 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.
> 
> 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 <namhyung@kernel.org>

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

Re: [RFC/PATCH] ftrace: Allow to change size of function graph filters

[Namhyung Kim]

Hi Steve,

On Fri, Jan 13, 2017 at 11:16:18AM -0500, Steven Rostedt wrote:
> On Fri, 13 Jan 2017 13:22:43 +0900
> Namhyung Kim <namhyung@kernel.org> 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.
> > 
> > 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 <namhyung@kernel.org>
> 
> 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.

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

Re: [RFC/PATCH] ftrace: Allow to change size of function graph filters

[Steven Rostedt]

On Sat, 14 Jan 2017 13:20:00 +0900
Namhyung Kim <namhyung@kernel.org> 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