[PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Chunyu Hu]

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@redhat.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

[PATCH 2/2] tracing: fix typoes in code comment and printk in trace_nop.c

[Chunyu Hu]

echo nop > /sys/kernel/debug/tracing/options/current_tracer
echo 1 > /sys/kernel/debug/tracing/options/test_nop_accept
echo 0 > /sys/kernel/debug/tracing/options/test_nop_accept
echo 1 > /sys/kernel/debug/tracing/options/test_nop_refuse

Before the fix, the dmesg is a bit ugly since a align issue.

[  191.973081] nop_test_accept flag set to 1: we accept. Now cat trace_options to see the result
[  195.156942] nop_test_refuse flag set to 1: we refuse.Now cat trace_options to see the result

After the fix, the dmesg will show aligned log for nop_test_refuse and nop_test_accept.

[ 2718.032413] nop_test_refuse flag set to 1: we refuse. Now cat trace_options to see the result
[ 2734.253360] nop_test_accept flag set to 1: we accept. Now cat trace_options to see the result

Signed-off-by: Chunyu Hu <chuhu@redhat.com>
---
 kernel/trace/trace_nop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_nop.c b/kernel/trace/trace_nop.c
index 8bb2071..49f61fe 100644
--- a/kernel/trace/trace_nop.c
+++ b/kernel/trace/trace_nop.c
@@ -56,7 +56,7 @@ static void nop_trace_reset(struct trace_array *tr)
 }
 
 /* It only serves as a signal handler and a callback to
- * accept or refuse tthe setting of a flag.
+ * accept or refuse the setting of a flag.
  * If you don't implement it, then the flag setting will be
  * automatically accepted.
  */
@@ -75,7 +75,7 @@ static int nop_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
 
 	if (bit == TRACE_NOP_OPT_REFUSE) {
 		printk(KERN_DEBUG "nop_test_refuse flag set to %d: we refuse."
-			"Now cat trace_options to see the result\n",
+			" Now cat trace_options to see the result\n",
 			set);
 		return -EINVAL;
 	}
-- 
1.8.3.1

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

Hi Chunyu,

[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.5-rc7 next-20160308]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chunyu-Hu/tracing-make-tracer_flags-use-the-right-set_flag-callback/20160308-214136
config: i386-randconfig-x000-201610 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/trace/trace.c: In function 'create_trace_option_files':
>> kernel/trace/trace.c:6403:7: error: 'i' undeclared (first use in this function)
     for (i = 0; i < tr->nr_topts; i++) {
          ^
   kernel/trace/trace.c:6403:7: note: each undeclared identifier is reported only once for each function it appears in

vim +/i +6403 kernel/trace/trace.c

37aea98b Steven Rostedt (Red Hat  2015-09-30  6397) 	 * If this is an instance, only create flags for tracers
37aea98b Steven Rostedt (Red Hat  2015-09-30  6398) 	 * the instance may have.
37aea98b Steven Rostedt (Red Hat  2015-09-30  6399) 	 */
37aea98b Steven Rostedt (Red Hat  2015-09-30  6400) 	if (!trace_ok_for_array(tracer, tr))
37aea98b Steven Rostedt (Red Hat  2015-09-30  6401) 		return;
37aea98b Steven Rostedt (Red Hat  2015-09-30  6402) 
37aea98b Steven Rostedt (Red Hat  2015-09-30 @6403) 	for (i = 0; i < tr->nr_topts; i++) {
37aea98b Steven Rostedt (Red Hat  2015-09-30  6404) 		/*
37aea98b Steven Rostedt (Red Hat  2015-09-30  6405) 		 * Check if these flags have already been added.
37aea98b Steven Rostedt (Red Hat  2015-09-30  6406) 		 * Some tracers share flags.

:::::: The code at line 6403 was first introduced by commit
:::::: 37aea98b84c0ce2ac638510fefeed9f8f920bd34 tracing: Add trace options for tracer options to instances

:::::: TO: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
:::::: CC: Steven Rostedt <rostedt@goodmis.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23390 bytes --]

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue,  8 Mar 2016 21:37:01 +0800
Chunyu Hu <chuhu@redhat.com> wrote:

> 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

Thanks for the report, I'll look into this.

> 
> 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.

Do *not* enable the option func_stack_trace without filtering
functions. It will cause *all* functions to perform a stack trace. It
doesn't actually hang the system, it just makes the system perform at
a Commodore 64 level. Where you may find yourself waiting a few minutes
to see any keystroke you make on the keyboard.

It's one of those "don't do this, unless you really enjoy the pain"
operations.

It usually takes me around 10 minutes to disable it after I
accidentally enable it.

-- Steve


> 
> 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 | 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 */

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue,  8 Mar 2016 21:37:01 +0800
Chunyu Hu <chuhu@redhat.com> wrote:

> @@ -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;

I'm guessing this was a mistake.

-- Steve

>  
>  	if (!tracer)
>  		return;

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue,  8 Mar 2016 21:37:01 +0800
Chunyu Hu <chuhu@redhat.com> wrote:

> 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.

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.

-- Steve

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

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) {

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Chunyu Hu]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: liwan@redhat.com, linux-kernel@vger.kernel.org
> Sent: Tuesday, March 8, 2016 10:33:17 PM
> Subject: Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback
> 
> On Tue,  8 Mar 2016 21:37:01 +0800
> Chunyu Hu <chuhu@redhat.com> wrote:
> 
> > @@ -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;
> 
> I'm guessing this was a mistake.

Thanks for the quick review! This is an mistake, the change below should be after it. From my
limited knowledge and observation, seems there is no tracer sharing the tracer_flag except the
dummy_tracer flag. As in the patch that flag is not shared any more, so i also removed these check
in the previous email.


@@ -6390,15 +6400,6 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
        if (!trace_ok_for_array(tracer, tr))
                return;
 
-       for (i = 0; i < tr->nr_topts; i++) {
-               /*
-                * Check if these flags have already been added.
-                * Some tracers share flags.
-                */
-               if (tr->topts[i].tracer->flags == tracer->flags)
-                       return;
-       }
-


> -- Steve
> 
> >  
> >  	if (!tracer)
> >  		return;
> 

-- 
Regards,
Chunyu Hu

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue, 8 Mar 2016 10:22:07 -0500 (EST)
Chunyu Hu <chuhu@redhat.com> wrote:


> Thanks for the quick review! This is an mistake, the change below should be after it. From my
> limited knowledge and observation, seems there is no tracer sharing the tracer_flag except the
> dummy_tracer flag. As in the patch that flag is not shared any more, so i also removed these check
> in the previous email.
> 
> 
> @@ -6390,15 +6400,6 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
>         if (!trace_ok_for_array(tracer, tr))
>                 return;
>  
> -       for (i = 0; i < tr->nr_topts; i++) {
> -               /*
> -                * Check if these flags have already been added.
> -                * Some tracers share flags.
> -                */
> -               if (tr->topts[i].tracer->flags == tracer->flags)
> -                       return;
> -       }
> -
> 

The latency tracers share flags (irqsoff, preemptoff and preemptirqsoff)

-- Steve

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue, 8 Mar 2016 10:32:11 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:


> The latency tracers share flags (irqsoff, preemptoff and preemptirqsoff)

Although, that may have changed recently. /me investigates.

-- Steve

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue, 8 Mar 2016 10:33:51 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 8 Mar 2016 10:32:11 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 
> > The latency tracers share flags (irqsoff, preemptoff and preemptirqsoff)  
> 
> Although, that may have changed recently. /me investigates.

Ah it did:

Commit 03905582fd0939 "tracing: Move "display-graph" option to main
options" removed duplicate trace options.

Thus, I guess your change is fine. I'll go back and include it.

-- Steve

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Chunyu Hu]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: liwan@redhat.com, linux-kernel@vger.kernel.org
> Sent: Tuesday, March 8, 2016 11:14:51 PM
> Subject: Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback
> 
> 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:

Sure! But my patch/build work are all manual, so I guess I can't keep with
your speed, but i will do. Thx.

> -- 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) {
> 
> 

-- 
Regards,
Chunyu Hu

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Tue, 8 Mar 2016 10:55:30 -0500 (EST)
Chunyu Hu <chuhu@redhat.com> wrote:


> Sure! But my patch/build work are all manual, so I guess I can't keep with
> your speed, but i will do. Thx.

Here's the latest version of your patch ;-)

-- Steve

>From d39cdd2036a63eef17a14efbd969405ca5612886 Mon Sep 17 00:00:00 2001
From: Chunyu Hu <chuhu@redhat.com>
Date: Tue, 8 Mar 2016 21:37:01 +0800
Subject: [PATCH] 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.

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.

Link: http://lkml.kernel.org/r/1457444222-8654-1-git-send-email-chuhu@redhat.com

Signed-off-by: Chunyu Hu <chuhu@redhat.com>
[ Fixed up function tracer options to work with the change ]
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c           | 28 ++++++++++++++--------------
 kernel/trace/trace.h           |  1 +
 kernel/trace/trace_functions.c |  6 ++++++
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d9293402ee68..b401a1892dc6 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);
@@ -6391,11 +6394,8 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
 		return;
 
 	for (i = 0; i < tr->nr_topts; i++) {
-		/*
-		 * Check if these flags have already been added.
-		 * Some tracers share flags.
-		 */
-		if (tr->topts[i].tracer->flags == tracer->flags)
+		/* Make sure there's no duplicate flags. */
+		if (WARN_ON_ONCE(tr->topts[i].tracer->flags == tracer->flags))
 			return;
 	}
 
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8414fa40bf27..b4cae47f283e 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 */
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index fcd41a166405..5a095c2e4b69 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -219,6 +219,8 @@ static void tracing_stop_function_trace(struct trace_array *tr)
 	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, u32 old_flags, u32 bit, int set)
 		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) {
-- 
1.8.3.1

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Chunyu Hu]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: liwan@redhat.com, linux-kernel@vger.kernel.org
> Sent: Wednesday, March 9, 2016 12:23:12 AM
> Subject: Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback
> 
> On Tue, 8 Mar 2016 10:55:30 -0500 (EST)
> Chunyu Hu <chuhu@redhat.com> wrote:
> 
> 
> > Sure! But my patch/build work are all manual, so I guess I can't keep with
> > your speed, but i will do. Thx.
> 
> Here's the latest version of your patch ;-)

Thanks. It's amazing, all the call trace  in ftrace.c are gone. I did
the ltp ftrace stress tests and some regression tests on it. I didn't hit 
any call trace. Thanks for the modification of the code. Just hit several
dmesg like: kernel: failed to start wakeup tracer. 

 
BTW, may I ask the question that what's the difference of func_stack_trace
and stacktrace? And Is there a case that the tracing_on can't be setup? I
haven't touch many parts of the code. Thanks ;)


> -- Steve
> 
> From d39cdd2036a63eef17a14efbd969405ca5612886 Mon Sep 17 00:00:00 2001
> From: Chunyu Hu <chuhu@redhat.com>
> Date: Tue, 8 Mar 2016 21:37:01 +0800
> Subject: [PATCH] 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.
> 
> 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.
> 
> Link:
> http://lkml.kernel.org/r/1457444222-8654-1-git-send-email-chuhu@redhat.com
> 
> Signed-off-by: Chunyu Hu <chuhu@redhat.com>
> [ Fixed up function tracer options to work with the change ]
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace.c           | 28 ++++++++++++++--------------
>  kernel/trace/trace.h           |  1 +
>  kernel/trace/trace_functions.c |  6 ++++++
>  3 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d9293402ee68..b401a1892dc6 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);
> @@ -6391,11 +6394,8 @@ create_trace_option_files(struct trace_array *tr,
> struct tracer *tracer)
>  		return;
>  
>  	for (i = 0; i < tr->nr_topts; i++) {
> -		/*
> -		 * Check if these flags have already been added.
> -		 * Some tracers share flags.
> -		 */
> -		if (tr->topts[i].tracer->flags == tracer->flags)
> +		/* Make sure there's no duplicate flags. */
> +		if (WARN_ON_ONCE(tr->topts[i].tracer->flags == tracer->flags))

Thanks! It's really a safer way to throw out this warn if there is duplicate.

>  			return;
>  	}
>  
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 8414fa40bf27..b4cae47f283e 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 */
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index fcd41a166405..5a095c2e4b69 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -219,6 +219,8 @@ static void tracing_stop_function_trace(struct
> trace_array *tr)
>  	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, u32 old_flags, u32
> bit, int set)
>  		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;
> +

At first I thought it should have some relation with the funcgraph trace. so i 
guessed was wrong. Thx.

>  		unregister_ftrace_function(tr->ops);
>  
>  		if (set) {
> --
> 1.8.3.1
> 
> 

-- 
Regards,
Chunyu Hu

Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback

[Steven Rostedt]

On Wed, 9 Mar 2016 08:38:45 -0500 (EST)
Chunyu Hu <chuhu@redhat.com> wrote:

> ----- Original Message -----
> > From: "Steven Rostedt" <rostedt@goodmis.org>
> > To: "Chunyu Hu" <chuhu@redhat.com>
> > Cc: liwan@redhat.com, linux-kernel@vger.kernel.org
> > Sent: Wednesday, March 9, 2016 12:23:12 AM
> > Subject: Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback
> > 
> > On Tue, 8 Mar 2016 10:55:30 -0500 (EST)
> > Chunyu Hu <chuhu@redhat.com> wrote:
> > 
> >   
> > > Sure! But my patch/build work are all manual, so I guess I can't keep with
> > > your speed, but i will do. Thx.  
> > 
> > Here's the latest version of your patch ;-)  
> 
> Thanks. It's amazing, all the call trace  in ftrace.c are gone. I did
> the ltp ftrace stress tests and some regression tests on it. I didn't hit 
> any call trace. Thanks for the modification of the code. Just hit several
> dmesg like: kernel: failed to start wakeup tracer. 
> 
>  
> BTW, may I ask the question that what's the difference of func_stack_trace
> and stacktrace? And Is there a case that the tracing_on can't be setup? I
> haven't touch many parts of the code. Thanks ;)


stacktrace works with events, func_stack_trace works with the function
tracer. They are separate for two reasons. One, they have completely
different infrastructures. Two, function tracing is far more invasive,
and enabling func_stack_trace without function filtering will pretty
much bring your system to a screaming halt.

I'm not sure what you mean about the tracing_on question. The
tracing_on file only connects to the tracing_on functionality in the
kernel, which when clear will stop the ring buffers from recording data.

-- Steve

> >  	for (i = 0; i < tr->nr_topts; i++) {
> > -		/*
> > -		 * Check if these flags have already been added.
> > -		 * Some tracers share flags.
> > -		 */
> > -		if (tr->topts[i].tracer->flags == tracer->flags)
> > +		/* Make sure there's no duplicate flags. */
> > +		if (WARN_ON_ONCE(tr->topts[i].tracer->flags == tracer->flags))  
> 
> Thanks! It's really a safer way to throw out this warn if there is duplicate.
> 
> >  			return;
> >  	}
> >  
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 8414fa40bf27..b4cae47f283e 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 */
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index fcd41a166405..5a095c2e4b69 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -219,6 +219,8 @@ static void tracing_stop_function_trace(struct
> > trace_array *tr)
> >  	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, u32 old_flags, u32
> > bit, int set)
> >  		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;
> > +  
> 
> At first I thought it should have some relation with the funcgraph trace. so i 
> guessed was wrong. Thx.
> 
> >  		unregister_ftrace_function(tr->ops);
> >  
> >  		if (set) {
> > --
> > 1.8.3.1
> > 
> >   
>