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