Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730157 > unrolled thread

[PATCH v3] tracing: Ignore mmiotrace from kernel commandline

Started by"Ziqian SUN (Zamir)" <zsun@redhat.com>
First post2017-09-11 08:30 +0200
Last post2017-09-19 18:30 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v3] tracing: Ignore mmiotrace from kernel commandline "Ziqian SUN (Zamir)" <zsun@redhat.com> - 2017-09-11 08:30 +0200
    Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline "Ziqian SUN (Zamir)" <zsun@redhat.com> - 2017-09-11 08:40 +0200
      Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline Steven Rostedt <rostedt@goodmis.org> - 2017-09-11 13:30 +0200
        Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline "Ziqian SUN (Zamir)" <zsun@redhat.com> - 2017-09-12 14:20 +0200
          Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline "Ziqian SUN (Zamir)" <zsun@redhat.com> - 2017-09-18 12:40 +0200
          Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline Steven Rostedt <rostedt@goodmis.org> - 2017-09-19 18:30 +0200

#1730157 — [PATCH v3] tracing: Ignore mmiotrace from kernel commandline

From"Ziqian SUN (Zamir)" <zsun@redhat.com>
Date2017-09-11 08:30 +0200
Subject[PATCH v3] tracing: Ignore mmiotrace from kernel commandline
Message-ID<uoqoh-7s2-1@gated-at.bofh.it>
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);
+		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

[toc] | [next] | [standalone]

#1730163

From"Ziqian SUN (Zamir)" <zsun@redhat.com>
Date2017-09-11 08:40 +0200
Message-ID<uoqxX-7vR-9@gated-at.bofh.it>
In reply to#1730157
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.
> +		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

[toc] | [prev] | [next] | [standalone]

#1730297

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-09-11 13:30 +0200
Message-ID<uov4B-2t7-9@gated-at.bofh.it>
In reply to#1730163
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

[toc] | [prev] | [next] | [standalone]

#1730836

From"Ziqian SUN (Zamir)" <zsun@redhat.com>
Date2017-09-12 14:20 +0200
Message-ID<uoSkx-17B-9@gated-at.bofh.it>
In reply to#1730297

On 09/11/2017 07:23 PM, Steven Rostedt wrote:
> 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.
> 

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

[toc] | [prev] | [next] | [standalone]

#1734015

From"Ziqian SUN (Zamir)" <zsun@redhat.com>
Date2017-09-18 12:40 +0200
Message-ID<ur1D4-59P-11@gated-at.bofh.it>
In reply to#1730836

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)" <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.
>>
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1735084

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-09-19 18:30 +0200
Message-ID<urtzk-7ID-9@gated-at.bofh.it>
In reply to#1730836
On Tue, 12 Sep 2017 20:18:35 +0800
"Ziqian SUN (Zamir)" <zsun@redhat.com> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web