Date: Tue, 8 Mar 2016 10:14:51 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Chunyu Hu <chuhu@redhat.com>
Cc: liwan@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag
 callback
Message-ID: <20160308101451.69f9bc18@gandalf.local.home>
In-Reply-To: <20160308095443.409de468@gandalf.local.home>
References: <1457444222-8654-1-git-send-email-chuhu@redhat.com>
	<20160308095443.409de468@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5538
Lines: 153

On Tue, 8 Mar 2016 09:54:43 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:


> > Here just forbit it return an invalid code to user space with extra
> > dmesg help info to avoid the complex WARN log.  
> 
> This is not acceptable. The whole point of making the options visible
> when the tracer is not active was to change the flags when the tracer
> is not active.
> 
> I'll look deeper into this. Thanks.

I made the modifications to your patch. Can you please test this. I'll
start running my own tests on it:

-- Steve

From: Chunyu Hu <chuhu@redhat.com>

tracing: Make tracer_flags use the right set_flag callback

When I was updating the ftrace_stress test of ltp. I encountered
a strange phenomemon, excute following steps:

echo nop > /sys/kernel/debug/tracing/current_tracer
echo 0 > /sys/kernel/debug/tracing/options/funcgraph-cpu
bash: echo: write error: Invalid argument

check dmesg:
[ 1024.903855] nop_test_refuse flag set to 0: we refuse.Now cat trace_options to see the result

The reason is that the trace option test will randomly setup trace
option under tracing/options no matter what the current_tracer is.
but the set_tracer_option is always using the set_flag callback
from the current_tracer. This patch adds a pointer to tracer_flags
and make it point to the tracer it belongs to. When the option is
setup, the set_flag of the right tracer will be used no matter
what the the current_tracer is.

But after some tests, find it's not easy to setup tracer flag when
its target is not the current tracer. Some check logic of function
and function_graph trace seems not appropriate now, some WARN in
ftrace.c are triggered.

kernel: WARNING: CPU: 2 PID: 5522 at kernel/trace/ftrace.c:5106 ftrace_init_array_ops+0x4a/0x70()
kernel: WARNING: CPU: 2 PID: 5522 at kernel/trace/ftrace.c:413 ftrace_startup+0x229/0x240()
kernel: WARNING: CPU: 2 PID: 30451 at kernel/trace/ftrace.c:460 return_to_handler+0x0/0x27()

Here just forbit it return an invalid code to user space with extra
dmesg help info to avoid the complex WARN log.

And the old dummy_tracer_flags is used for all the tracers which
doesn't have a tracer_flags, having issue to use it to save the
pointer of a tracer. So remove it and use dynamic dummy tracer_flags
for tracers needing a dummy tracer_flags, as a result, there are no
tracers sharing tracer_flags, so remove the check code.

And save the current tracer to trace_option_dentry seems not good as
it may waste mem space when mount the debug/trace fs more than one time.

Signed-off-by: Chunyu Hu <chuhu@redhat.com>
---
 kernel/trace/trace.c           |   21 ++++++++++++---------
 kernel/trace/trace.h           |    1 +
 kernel/trace/trace_functions.c |    6 ++++++
 3 files changed, 19 insertions(+), 9 deletions(-)

Index: linux-trace.git/kernel/trace/trace.c
===================================================================
--- linux-trace.git.orig/kernel/trace/trace.c	2016-03-08 10:07:51.180345420 -0500
+++ linux-trace.git/kernel/trace/trace.c	2016-03-08 10:07:54.365296167 -0500
@@ -74,11 +74,6 @@ static struct tracer_opt dummy_tracer_op
 	{ }
 };
 
-static struct tracer_flags dummy_tracer_flags = {
-	.val = 0,
-	.opts = dummy_tracer_opt
-};
-
 static int
 dummy_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
 {
@@ -1258,12 +1253,20 @@ int __init register_tracer(struct tracer
 
 	if (!type->set_flag)
 		type->set_flag = &dummy_set_flag;
-	if (!type->flags)
-		type->flags = &dummy_tracer_flags;
-	else
+	if (!type->flags) {
+		/*allocate a dummy tracer_flags*/
+		type->flags = kmalloc(sizeof(*type->flags), GFP_KERNEL);
+		if (!type->flags)
+			return -ENOMEM;
+		type->flags->val = 0;
+		type->flags->opts = dummy_tracer_opt;
+	} else
 		if (!type->flags->opts)
 			type->flags->opts = dummy_tracer_opt;
 
+	/* store the tracer for __set_tracer_option */
+	type->flags->trace = type;
+
 	ret = run_tracer_selftest(type);
 	if (ret < 0)
 		goto out;
@@ -3505,7 +3508,7 @@ static int __set_tracer_option(struct tr
 			       struct tracer_flags *tracer_flags,
 			       struct tracer_opt *opts, int neg)
 {
-	struct tracer *trace = tr->current_trace;
+	struct tracer *trace = tracer_flags->trace;
 	int ret;
 
 	ret = trace->set_flag(tr, tracer_flags->val, opts->bit, !neg);
Index: linux-trace.git/kernel/trace/trace.h
===================================================================
--- linux-trace.git.orig/kernel/trace/trace.h	2016-03-08 10:07:51.180345420 -0500
+++ linux-trace.git/kernel/trace/trace.h	2016-03-08 10:07:54.366296151 -0500
@@ -345,6 +345,7 @@ struct tracer_opt {
 struct tracer_flags {
 	u32			val;
 	struct tracer_opt	*opts;
+	struct tracer		*trace;
 };
 
 /* Makes more easy to define a tracer opt */
Index: linux-trace.git/kernel/trace/trace_functions.c
===================================================================
--- linux-trace.git.orig/kernel/trace/trace_functions.c	2016-03-08 10:07:35.413589131 -0500
+++ linux-trace.git/kernel/trace/trace_functions.c	2016-03-08 10:08:03.030162132 -0500
@@ -219,6 +219,8 @@ static void tracing_stop_function_trace(
 	unregister_ftrace_function(tr->ops);
 }
 
+static struct tracer function_trace;
+
 static int
 func_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
 {
@@ -228,6 +230,10 @@ func_set_flag(struct trace_array *tr, u3
 		if (!!set == !!(func_flags.val & TRACE_FUNC_OPT_STACK))
 			break;
 
+		/* We can change this flag when not running. */
+		if (tr->current_trace != &function_trace)
+			break;
+
 		unregister_ftrace_function(tr->ops);
 
 		if (set) {