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

[Chunyu Hu <chuhu@redhat.com>]

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