From: "Ziqian SUN (Zamir)" <[email protected]>
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) <[email protected]>
--
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);
+ goto out;
+ }
+
/* Some tracers are only allowed for the top level buffer */
if (!trace_ok_for_array(t, tr)) {
ret = -EINVAL;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index fb5d54d..652c682 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -444,6 +444,8 @@ struct tracer {
#ifdef CONFIG_TRACER_MAX_TRACE
bool use_max_tr;
#endif
+ /* True if tracer cannot be enabled in kernel param */
+ bool noboot;
};
diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index cd7480d..dca78fc 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -282,6 +282,7 @@ static enum print_line_t mmio_print_line(struct trace_iterator *iter)
.close = mmio_close,
.read = mmio_read,
.print_line = mmio_print_line,
+ .noboot = true,
};
__init static int init_mmio_trace(void)
--
1.8.3.1
On 09/11/2017 02:26 PM, Ziqian SUN (Zamir) wrote:
> From: "Ziqian SUN (Zamir)" <[email protected]>
>
> 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) <[email protected]>
> --
> 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.
> + goto out;
> + }
> +
> /* Some tracers are only allowed for the top level buffer */
> if (!trace_ok_for_array(t, tr)) {
> ret = -EINVAL;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index fb5d54d..652c682 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -444,6 +444,8 @@ struct tracer {
> #ifdef CONFIG_TRACER_MAX_TRACE
> bool use_max_tr;
> #endif
> + /* True if tracer cannot be enabled in kernel param */
> + bool noboot;
> };
>
>
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index cd7480d..dca78fc 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -282,6 +282,7 @@ static enum print_line_t mmio_print_line(struct trace_iterator *iter)
> .close = mmio_close,
> .read = mmio_read,
> .print_line = mmio_print_line,
> + .noboot = true,
> };
>
> __init static int init_mmio_trace(void)
>
This patch passed my own test on top of Fedora kernel 4.13.0-1.fc26.x86_64.
--
Ziqian SUN (Zamir)
9F Raycom office (NAY)
Red Hat Software (Beijing) Co.,Ltd
IRC: zsun (internal and freenode)
Tel: +86 10 65627458
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E
On Mon, 11 Sep 2017 14:31:34 +0800
"Ziqian SUN (Zamir)" <[email protected]> wrote:
> On 09/11/2017 02:26 PM, Ziqian SUN (Zamir) wrote:
> > From: "Ziqian SUN (Zamir)" <[email protected]>
> >
> > 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) <[email protected]>
> > --
> > 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
On 09/11/2017 07:23 PM, Steven Rostedt wrote:
> On Mon, 11 Sep 2017 14:31:34 +0800
> "Ziqian SUN (Zamir)" <[email protected]> wrote:
>
>> On 09/11/2017 02:26 PM, Ziqian SUN (Zamir) wrote:
>>> From: "Ziqian SUN (Zamir)" <[email protected]>
>>>
>>> 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) <[email protected]>
>>> --
>>> 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.
>
Following is the log I tried with WARN before. With WARN we have both
the warning message, and the stack trace. The stack trace goes into
stacing_set_tracer, but not showing mmiotrace. I mean, this is useful
for debugging, but maybe confusing for users who is willing to report
bugs but not a kernel geek. With pr_warn we already indicate mmiotrace
is ignored, and is neat.
Just my cents.
[ 7.134149] Starting tracer 'mmiotrace'
[ 7.134151] Tracer 'mmiotrace' is not allowed on command line
[ 7.134162] ------------[ cut here ]------------
[ 7.134171] WARNING: CPU: 6 PID: 1 at kernel/trace/trace.c:5354
tracing_set_tracer+0x168/0x1b0
[ 7.134171] Modules linked in:
[ 7.134176] CPU: 6 PID: 1 Comm: swapper/0 Not tainted
4.13.0-1.mmiotracev3.fc26.x86_64 #1
[ 7.134178] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 09/18/2013
[ 7.134179] task: ffff94b7310d8000 task.stack: ffffaadc83144000
[ 7.134183] RIP: 0010:tracing_set_tracer+0x168/0x1b0
[ 7.134185] RSP: 0000:ffffaadc83147da0 EFLAGS: 00010286
[ 7.134187] RAX: 0000000000000031 RBX: ffffffffad04e340 RCX:
0000000000000000
[ 7.134189] RDX: 0000000000000000 RSI: 0000000000000002 RDI:
0000000000000246
[ 7.134190] RBP: ffffaadc83147dc8 R08: 0000000000000031 R09:
ffffffffad314f00
[ 7.134191] R10: ffffaadc83147d30 R11: 0000000000000000 R12:
0000000000000000
[ 7.134193] R13: ffffffffacc935f3 R14: ffffffffacc935f3 R15:
ffffffffacef46c0
[ 7.134196] FS: 0000000000000000(0000) GS:ffff94b732e00000(0000)
knlGS:0000000000000000
[ 7.134197] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 7.134199] CR2: 0000000000000000 CR3: 00000005f5e09000 CR4:
00000000000406e0
[ 7.134201] Call Trace:
[ 7.134210] ? set_debug_rodata+0x17/0x17
[ 7.134213] register_tracer+0x1a1/0x1c2
[ 7.134216] ? stack_trace_init+0xb5/0xb5
[ 7.134218] init_mmio_trace+0x10/0x12
[ 7.134222] do_one_initcall+0x50/0x190
[ 7.134225] kernel_init_freeable+0x1a8/0x245
[ 7.134231] ? rest_init+0xc0/0xc0
[ 7.134234] kernel_init+0xe/0x101
[ 7.134238] ret_from_fork+0x25/0x30
[ 7.134240] Code: ff e8 6d d0 ff ff 85 c0 41 89 c4 79 8e eb b7 80 bb
9b 00 00 00 00 0f 84 26 ff ff ff 4c 89 ee 48 c7 c7 58 1e c9 ac e8 99 a0
f8 ff <0f> ff eb 97 48 8b 35 9d 81 d7 00 ba ff ff ff ff 4c 89 ff e8 10
[ 7.134279] ---[ end trace 978ff599b6e516bc ]---
> 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
>
--
Ziqian SUN (Zamir)
9F Raycom office (NAY)
Red Hat Software (Beijing) Co.,Ltd
IRC: zsun (internal and freenode)
Tel: +86 10 65627458
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E
On 09/12/2017 08:18 PM, Ziqian SUN (Zamir) wrote:
>
>
> On 09/11/2017 07:23 PM, Steven Rostedt wrote:
>> On Mon, 11 Sep 2017 14:31:34 +0800
>> "Ziqian SUN (Zamir)" <[email protected]> wrote:
>>
>>> On 09/11/2017 02:26 PM, Ziqian SUN (Zamir) wrote:
>>>> From: "Ziqian SUN (Zamir)" <[email protected]>
>>>>
>>>> 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) <[email protected]>
>>>> --
>>>> 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.
>>
>
> Following is the log I tried with WARN before. With WARN we have both
> the warning message, and the stack trace. The stack trace goes into
> stacing_set_tracer, but not showing mmiotrace. I mean, this is useful
> for debugging, but maybe confusing for users who is willing to report
> bugs but not a kernel geek. With pr_warn we already indicate mmiotrace
> is ignored, and is neat.
>
> Just my cents.
>
> [ 7.134149] Starting tracer 'mmiotrace'
> [ 7.134151] Tracer 'mmiotrace' is not allowed on command line
> [ 7.134162] ------------[ cut here ]------------
> [ 7.134171] WARNING: CPU: 6 PID: 1 at kernel/trace/trace.c:5354
> tracing_set_tracer+0x168/0x1b0
> [ 7.134171] Modules linked in:
> [ 7.134176] CPU: 6 PID: 1 Comm: swapper/0 Not tainted
> 4.13.0-1.mmiotracev3.fc26.x86_64 #1
> [ 7.134178] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 09/18/2013
> [ 7.134179] task: ffff94b7310d8000 task.stack: ffffaadc83144000
> [ 7.134183] RIP: 0010:tracing_set_tracer+0x168/0x1b0
> [ 7.134185] RSP: 0000:ffffaadc83147da0 EFLAGS: 00010286
> [ 7.134187] RAX: 0000000000000031 RBX: ffffffffad04e340 RCX:
> 0000000000000000
> [ 7.134189] RDX: 0000000000000000 RSI: 0000000000000002 RDI:
> 0000000000000246
> [ 7.134190] RBP: ffffaadc83147dc8 R08: 0000000000000031 R09:
> ffffffffad314f00
> [ 7.134191] R10: ffffaadc83147d30 R11: 0000000000000000 R12:
> 0000000000000000
> [ 7.134193] R13: ffffffffacc935f3 R14: ffffffffacc935f3 R15:
> ffffffffacef46c0
> [ 7.134196] FS: 0000000000000000(0000) GS:ffff94b732e00000(0000)
> knlGS:0000000000000000
> [ 7.134197] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 7.134199] CR2: 0000000000000000 CR3: 00000005f5e09000 CR4:
> 00000000000406e0
> [ 7.134201] Call Trace:
> [ 7.134210] ? set_debug_rodata+0x17/0x17
> [ 7.134213] register_tracer+0x1a1/0x1c2
> [ 7.134216] ? stack_trace_init+0xb5/0xb5
> [ 7.134218] init_mmio_trace+0x10/0x12
> [ 7.134222] do_one_initcall+0x50/0x190
> [ 7.134225] kernel_init_freeable+0x1a8/0x245
> [ 7.134231] ? rest_init+0xc0/0xc0
> [ 7.134234] kernel_init+0xe/0x101
> [ 7.134238] ret_from_fork+0x25/0x30
> [ 7.134240] Code: ff e8 6d d0 ff ff 85 c0 41 89 c4 79 8e eb b7 80 bb
> 9b 00 00 00 00 0f 84 26 ff ff ff 4c 89 ee 48 c7 c7 58 1e c9 ac e8 99 a0
> f8 ff <0f> ff eb 97 48 8b 35 9d 81 d7 00 ba ff ff ff ff 4c 89 ff e8 10
> [ 7.134279] ---[ end trace 978ff599b6e516bc ]---
>
>> 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.
Ping for updates.
--
Ziqian SUN (Zamir)
9F Raycom office (NAY)
Red Hat Software (Beijing) Co.,Ltd
IRC: zsun (internal and freenode)
Tel: +86 10 65627458
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E
On Tue, 12 Sep 2017 20:18:35 +0800
"Ziqian SUN (Zamir)" <[email protected]> wrote:
> >> 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.
> >
>
> Following is the log I tried with WARN before. With WARN we have both
> the warning message, and the stack trace. The stack trace goes into
> stacing_set_tracer, but not showing mmiotrace. I mean, this is useful
> for debugging, but maybe confusing for users who is willing to report
> bugs but not a kernel geek. With pr_warn we already indicate mmiotrace
> is ignored, and is neat.
>
I'm now leaning towards WARN() over pr_warn(). Sure it's more useful
for a kernel geek and not an everyday user, but I will counter that an
everyday user should most definitely not be enabling mmiotrace! Or
other traces for that matter.
But then again, it's not really a bug (it's a feature), so I guess I'll
just take this patch as is.
-- Steve