Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753810AbcCHNhV (ORCPT ); Tue, 8 Mar 2016 08:37:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35087 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751384AbcCHNhN (ORCPT ); Tue, 8 Mar 2016 08:37:13 -0500 From: Chunyu Hu To: rostedt@goodmis.org Cc: liwan@redhat.com, linux-kernel@vger.kernel.org Subject: [PATCH 1/2] tracing: make tracer_flags use the right set_flag callback Date: Tue, 8 Mar 2016 21:37:01 +0800 Message-Id: <1457444222-8654-1-git-send-email-chuhu@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4722 Lines: 135 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 --- 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