Date: Mon, 11 Sep 2017 07:23:21 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: "Ziqian SUN (Zamir)" <zsun@redhat.com>
Cc: linux-kernel@vger.kernel.org, mingo@redhat.com, karolherbst@gmail.com,
        ppaalanen@gmail.com, akpm@linux-foundation.org
Subject: Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline
Message-ID: <20170911072321.0085db2c@vmware.local.home>
In-Reply-To: <3cec495b-89a5-b8c6-e319-af9f969582e0@redhat.com>
References: <1505111195-31942-1-git-send-email-zsun@redhat.com>
        <3cec495b-89a5-b8c6-e319-af9f969582e0@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1950
Lines: 47

On Mon, 11 Sep 2017 14:31:34 +0800
"Ziqian SUN (Zamir)" <zsun@redhat.com> wrote:

> On 09/11/2017 02:26 PM, Ziqian SUN (Zamir) wrote:
> > From: "Ziqian SUN (Zamir)" <zsun@redhat.com>
> > 
> > The mmiotrace tracer cannot be enabled with ftrace=mmiotrace in kernel
> > commandline. With this patch, noboot is added to the tracer struct,
> > and when system boot with a tracer that has noboot=true, it will print
> > out a warning message and continue booting.
> > 
> > Signed-off-by: Ziqian SUN (Zamir) <zsun@redhat.com>
> > --
> > v1 -> v2 : remove unessential comment
> > v2 -> v3 : Use tracer struct instead of a separate list to store noboot
> > ---
> >   kernel/trace/trace.c           | 7 +++++++
> >   kernel/trace/trace.h           | 2 ++
> >   kernel/trace/trace_mmiotrace.c | 1 +
> >   3 files changed, 10 insertions(+)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 5360b7a..48c474f 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -5358,6 +5358,13 @@ static int tracing_set_tracer(struct trace_array *tr, const char *buf)
> >   	if (t == tr->current_trace)
> >   		goto out;
> >   
> > +	/* Some tracers won't work on kernel command line */
> > +	if (system_state < SYSTEM_RUNNING && t->noboot) {
> > +		pr_warn("Tracer '%s' is not allowed on command line, ignored\n",
> > +			t->name);  
> I feel the core trace printed by WARN is not so meaningful, so I use 
> pr_warn instead.

I'm fine with pr_warn, but I'm curious to what you mean by "not so
meaningful"? A WARN() will cause a dump stack, which usually shows up
as a bug in systems and more likely to be seen. But if someone is
adding this to the kernel command line and it's not working, they
should be looking for the tracer name within the dmesg anyway.

Also, I'm currently at OSS in LA and hopefully I don't lose this patch.
If you don't see anything by next Monday from me, feel free to send me a
reminder ping.

-- Steve