Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932721AbcCHPzo (ORCPT ); Tue, 8 Mar 2016 10:55:44 -0500 Received: from mx3-phx2.redhat.com ([209.132.183.24]:37080 "EHLO mx3-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932628AbcCHPze (ORCPT ); Tue, 8 Mar 2016 10:55:34 -0500 Date: Tue, 8 Mar 2016 10:55:30 -0500 (EST) From: Chunyu Hu Reply-To: Chunyu Hu To: Steven Rostedt Cc: liwan@redhat.com, linux-kernel@vger.kernel.org Message-ID: <587988151.26798768.1457452530893.JavaMail.zimbra@redhat.com> In-Reply-To: <20160308101451.69f9bc18@gandalf.local.home> References: <1457444222-8654-1-git-send-email-chuhu@redhat.com> <20160308095443.409de468@gandalf.local.home> <20160308101451.69f9bc18@gandalf.local.home> Subject: Re: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.68.5.41] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF38 (Linux)/8.0.6_GA_5922) Thread-Topic: tracing: make tracer_flags use the right set_flag callback Thread-Index: aLOCXuoicGcE7NpDGoxX/FVVesWcdQ== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6294 Lines: 179 ----- Original Message ----- > From: "Steven Rostedt" > To: "Chunyu Hu" > 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 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 > > 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 > --- > 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