Message-Id: <1457444222-8654-1-git-send-email-chuhu@redhat.com>
Date:	Tue,  8 Mar 2016 21:37:01 +0800
From:	Chunyu Hu <chuhu@...hat.com>
To:	rostedt@...dmis.org
Cc:	liwan@...hat.com, linux-kernel@...r.kernel.org
Subject: [PATCH 1/2] 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.

Another issue is the lockup issue, After setting all options to 1,
then setup the function trace, then the system will hang. That is
not the one this patch can fix.

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@...hat.com>
---
 kernel/trace/trace.c | 30 ++++++++++++++++++++----------
 kernel/trace/trace.h |  1 +
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8a98134..854dba5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -74,11 +74,6 @@ static struct tracer_opt dummy_tracer_opt[] = {
 	{ }
 };
 
-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 *type)
 
 	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 trace_array *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);
@@ -6196,6 +6199,14 @@ trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (val != 0 && val != 1)
 		return -EINVAL;
 
+	if (topt->flags->trace != topt->tr->current_trace) {
+		printk(KERN_DEBUG "Tracer flag %s is for %s trace,"
+			" first please set the current tracer to %s\n",
+			topt->opt->name, topt->flags->trace->name,
+			topt->flags->trace->name);
+		return -EINVAL;
+	}
+
 	if (!!(topt->flags->val & topt->opt->bit) != val) {
 		mutex_lock(&trace_types_lock);
 		ret = __set_tracer_option(topt->tr, topt->flags,
@@ -6373,7 +6384,6 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
 	struct tracer_flags *flags;
 	struct tracer_opt *opts;
 	int cnt;
-	int i;
 
 	if (!tracer)
 		return;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8414fa4..b4cae47 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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 */
-- 
1.8.3.1

Powered by blists - more mailing lists