Date:   Mon, 18 Sep 2017 18:34:11 +0800
From:   "Ziqian SUN (Zamir)" <zsun@...hat.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, mingo@...hat.com,
        karolherbst@...il.com, ppaalanen@...il.com,
        akpm@...ux-foundation.org
Subject: Re: [PATCH v3] tracing: Ignore mmiotrace from kernel commandline



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@...hat.com> wrote:
>>
>>> On 09/11/2017 02:26 PM, Ziqian SUN (Zamir) wrote:
>>>> From: "Ziqian SUN (Zamir)" <zsun@...hat.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@...hat.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

Powered by blists - more mailing lists