From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
Ondrej Mosnacek <omosnace@redhat.com>,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH] twist: allow converting pr_devel()/pr_debug() into printk(KERN_DEBUG)
Date: Wed, 27 May 2020 19:13:38 +0900 [thread overview]
Message-ID: <35d76737-8d23-9fb2-8e55-507109317f44@i-love.sakura.ne.jp> (raw)
In-Reply-To: <20200527083747.GA27273@linux-b0ei>
On 2020/05/27 17:37, Petr Mladek wrote:
> On Mon 2020-05-25 19:43:04, Tetsuo Handa wrote:
>> On 2020/05/25 17:42, Petr Mladek wrote:
>>> I see few drawbacks with this patch:
>>>
>>> 1. It will cause adding much more messages into the logbuffer even
>>> though they are not flushed to the console. It might cause that
>>> more important messages will get overridden before they reach
>>> console. They might also make hard to read the full log.
>>
>> Since the user of this twist option will select console loglevel in a way
>> KERN_DEBUG messages are not printed to consoles, KERN_DEBUG messages will
>> be immediately processed (and space for future messages will be reclaimed).
>> Therefore, I don't think that more important messages will get overridden.
>
> This is not fully true. More important messages will still be printed
> to the console. The debug messages will not be skipped before the
> older messages are proceed.
>
> I mean that many debug messages might cause losing more important ones
> before the old important messages are proceed.
Then, this reasoning will be also applicable to
[PATCH] printk: Add loglevel for "do not print to consoles".
in a sense that "don't try to quickly queue a lot of messages" rule. This concern
cannot be solved even if asynchronous printk() and per console loglevel are
supported someday, and oom_dump_tasks() is not allowed to count on these for
solving the stall problem caused by reporting all OOM victim candidates at once.
>
>
>> This twist option might increase possibility of mixing KERN_DEBUG messages
>> and non-KERN_DEBUG messages due to KERN_CONT case.
>>
>> But if these concerns turn out to be a real problem, we can redirect
>> pr_devel()/pr_debug() to simple snprintf() which evaluates arguments
>> but discards the result without storing into the logbuffer.
>>
>>>
>>> 2. Crash inside printk() causes recursive messages. They are currently
>>> printed into the printk_safe() buffers and there is a bigger risk
>>> that they will not reach the console.
>>
>> Currently "static char textbuf[LOG_LINE_MAX];" is "static" because it is used
>> under logbuf_lock. If we remove "static", we can use "char textbuf[LOG_LINE_MAX];"
>> without logbuf_lock. Then, we can bring potentially dangerous-and-slow vscnprintf()
>> in vprintk_store() to earlier stage (and vprintk_store() will need to do simple
>> copy) so that oops in printk() will happen before entering printk-safe context.
>> I think that this change follows a direction which lockless logbuf will want.
>
> No, LOG_LINE_MAX is too big to be allocated on stack.
We could assign per task_struct buffers and per CPU interrupt context buffers
(like we discussed about how to handle KERN_CONT problem). But managing these
off stack buffers is out of scope for this patch.
>
> Well, it would be possible to call vsprintf() with NULL buffer. It is
> normally used to calculate the length of the message before it is
> printed. But it also does all the accesses without printing anything.
OK. I think that redirecting pr_debug() to vsnprintf(NULL, 0) will be
better than modifying dynamic_debug path, for
>
>
>>> Have you tested this patch by the syzcaller with many runs, please?
>>> Did it helped to actually discover more bugs?
>>> Did it really made things easier?
>>
>> syzbot can't test with custom patches. The only way to test this patch is
>> to send to e.g. linux-next.git which syzbot is testing.
>
> OK, we could try this via some test branch that will go into
> linux-next but it would not be scheduled for the next merge window.
>
> For the testing, this patch might be good enough.
>
> For eventual upstreaming, I would prefer to handle this in
> lib/dynamic_debug.c by enabling all entries by default. This
> would solve all DYNAMIC_DEBUG_BRANCH() users at one place.
since "enabling all entries by default" will redirect pr_debug() calls to
printk(KERN_DEBUG), the "don't try to quickly queue too much messages" above
remains (i.e. it is essentially same with what this patch is doing).
>
> Anyway, I would like to see a proof that it really helped to find
> some bugs an easier way before upstreaming.
>
> Best Regards,
> Petr
>
next prev parent reply other threads:[~2020-05-27 10:13 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-24 14:50 [PATCH] twist: allow converting pr_devel()/pr_debug() into printk(KERN_DEBUG) Tetsuo Handa
2020-05-24 17:38 ` Joe Perches
2020-05-24 19:18 ` Ondrej Mosnacek
2020-05-25 5:03 ` Tetsuo Handa
2020-05-25 6:07 ` Joe Perches
2020-05-25 7:38 ` Dmitry Vyukov
2020-05-25 8:42 ` Petr Mladek
2020-05-25 9:11 ` Sergey Senozhatsky
2020-05-25 10:43 ` Tetsuo Handa
2020-05-27 8:37 ` Petr Mladek
2020-05-27 10:13 ` Tetsuo Handa [this message]
2020-05-27 15:55 ` Petr Mladek
2020-05-27 23:33 ` Tetsuo Handa
2020-05-28 6:56 ` [PATCH v2] twist: allow converting pr_devel()/pr_debug() into snprintf() Tetsuo Handa
2020-05-28 11:06 ` Petr Mladek
2020-05-28 15:16 ` Tetsuo Handa
2020-05-28 19:10 ` Andrew Morton
2020-05-28 19:50 ` Linus Torvalds
2020-05-28 20:01 ` Linus Torvalds
2020-05-29 0:07 ` Tetsuo Handa
2020-05-29 0:28 ` Linus Torvalds
2020-05-29 2:13 ` Tetsuo Handa
2020-05-29 2:24 ` Linus Torvalds
2020-05-29 4:47 ` Tetsuo Handa
2020-05-29 13:26 ` Tetsuo Handa
2020-06-03 11:03 ` twist: allow disabling reboot request Tetsuo Handa
2020-06-03 12:44 ` Petr Mladek
2020-06-03 13:35 ` Tetsuo Handa
2020-06-04 10:21 ` Petr Mladek
2020-06-08 7:48 ` [PATCH v2] twist: allow converting pr_devel()/pr_debug() into snprintf() Dmitry Vyukov
2020-06-08 10:30 ` Tetsuo Handa
2020-06-08 11:31 ` Andrey Konovalov
2020-05-29 8:17 ` Petr Mladek
2020-06-08 16:39 ` Geert Uytterhoeven
2020-05-28 10:59 ` [PATCH] twist: allow converting pr_devel()/pr_debug() into printk(KERN_DEBUG) Petr Mladek
2020-05-28 11:33 ` Tetsuo Handa
2020-05-28 12:14 ` Petr Mladek
2020-05-28 14:13 ` Tetsuo Handa
2020-05-28 17:08 ` Joe Perches
2020-05-29 2:04 ` Sergey Senozhatsky
2020-05-29 5:06 ` Tetsuo Handa
2020-05-27 9:59 ` kbuild test robot
2020-05-27 13:41 ` Tetsuo Handa
2020-05-27 12:37 ` kbuild test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=35d76737-8d23-9fb2-8e55-507109317f44@i-love.sakura.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=akpm@linux-foundation.org \
--cc=dvyukov@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=omosnace@redhat.com \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).