All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Dmitriy Vyukov <dvyukov@google.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Alexander Potapenko <glider@google.com>,
	Fengguang Wu <fengguang.wu@intel.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH 3/3] lockdep: Use line-buffered printk() for lockdep messages.
Date: Sat, 10 Nov 2018 11:42:03 +0900	[thread overview]
Message-ID: <deb8d78b-0593-2b8e-1c7a-9203aa77005f@i-love.sakura.ne.jp> (raw)
In-Reply-To: <20181109154326.apqkbsojmbg26o3b@pathway.suse.cz>

On 2018/11/10 0:43, Petr Mladek wrote:
>> + * Line buffered printk() tries to assign a buffer when printk() from a new
>> + * context identifier comes in. And it automatically releases that buffer when
>> + * one of three conditions listed below became true.
>> + *
>> + *   (1) printk() from that context identifier emitted '\n' as the last
>> + *       character of output.
>> + *   (2) printk() from that context identifier tried to print a too long line
>> + *       which cannot be stored into a buffer.
>> + *   (3) printk() from a new context identifier noticed that some context
>> + *       identifier is reserving a buffer for more than 10 seconds without
>> + *       emitting '\n'.
>> + *
>> + * Since (3) is based on a heuristic that somebody forgot to emit '\n' as the
>> + * last character of output(), pr_cont()/KERN_CONT users are expected to emit
>> + * '\n' within 10 seconds even if they reserved a buffer.
> 
> This is my main concern about this approach. It is so easy to omit
> the final '\n'.

If it is so easy to forget the final '\n', there will be a lot of implicit
pr_cont() users (because pr_cont() assumes that previous printk() omitted the
final '\n'), and "I am going to investigate much more pr_cont() users." will
be insufficient for getting meaningful conclusion.

Checking "lack of the the final '\n'" means that we need to check
"all printk() users who are not emitting the final '\n'" and evaluate
"whether there is a possibility that subsequent printk() will not be
 called from that context due to e.g. conditional branches". That is an
impossible task for anybody, for there might be out-of-tree code doing it.

> 
> They are currently delayed until another printk(). Even this is bad.
> Unfortunately we could not setup timer from printk() because it
> would add more locks into the game.

We could use interval timer for flushing incomplete line.
But updating printk() users to always end with '\n' will be preferable.

> 
> This patch will make it worse. They might get delayed by 10s or even
> more. Many other lines might appear in between. Also the code is
> more tricky[*].

If there are a lot of implicit pr_cont() users (who are omitting
the final '\n' in previous printk()), we can try to find them (and
fix them) by "reporting the location of printk() which omitted the
final '\n'" instead of "flushing the partial line from different
processes" when "try_buffered_printk() (or an interval timer) found
that some buffer is holding a partial line for more than 10 seconds".

> 
> 
> Sign, I am really unhappy how the buffered_printk() conversion
> looks in lockdep.c. But I still think that the approach is more
> reliable. I am going to investigate much more pr_cont() users.

If there is a possibility that subsequent printk() will not be called from
that context due to e.g. conditional branches, we will need to flush after
previous printk() in order to eliminate possibility of failing to flush.

That is asking printk() users to fix previous printk() so that partial
line is always flushed (with '\n' or without '\n').

> I wonder how many would be that complicated. I do not want
> to give up just because one use-case that was complicated
> even before.
> 
> 
> [*] The buffer can get written and flushed by different processes.
>     It is not trivial to do it correctly a lockless way.
> 
>     The proposed locking looks right on the first glance. But
>     the code is a bit scary ;-)

try_buffered_printk() will be a good hook for finding implicit
pr_cont() users who are omitting the final '\n', for we can find them
without making changes to printk() users.

try_buffered_printk() hook can offload atomic-but-complicated task
(converting e.g. %pSOMETHING to %s) to outside of logbuf_lock, and
reduce the period of holding logbuf_lock.

Since try_buffered_printk() serves as a hook, if we ignore the offloading
atomic-but-complicated task, we can eliminate try_buffered_printk() after
we updated printk() users to always end with '\n'.

  reply	other threads:[~2018-11-10  2:42 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-02 13:31 [PATCH v6 1/3] printk: Add line-buffered printk() API Tetsuo Handa
2018-11-02 13:31 ` [PATCH 2/3] mm: Use line-buffered printk() for show_free_areas() Tetsuo Handa
2018-11-07 14:07   ` Petr Mladek
2018-11-02 13:31 ` [PATCH 3/3] lockdep: Use line-buffered printk() for lockdep messages Tetsuo Handa
2018-11-02 13:36   ` Peter Zijlstra
2018-11-03  2:00     ` Tetsuo Handa
2018-11-06  8:38     ` Petr Mladek
2018-11-06  9:05       ` Sergey Senozhatsky
2018-11-06 12:57         ` Petr Mladek
2018-11-06  9:56       ` Tetsuo Handa
2018-11-07 15:19   ` Petr Mladek
2018-11-08  4:45     ` Sergey Senozhatsky
2018-11-08 11:37       ` Tetsuo Handa
2018-11-09  6:12         ` Sergey Senozhatsky
2018-11-09  9:55           ` Tetsuo Handa
2018-11-09 15:43             ` Petr Mladek
2018-11-10  2:42               ` Tetsuo Handa [this message]
2018-11-23 12:46                 ` Petr Mladek
2018-11-23 13:12                   ` Tetsuo Handa
2018-11-23 15:56                   ` Steven Rostedt
2018-11-24  0:24                     ` Tetsuo Handa
2018-11-26  4:34                       ` Sergey Senozhatsky
2018-11-28 13:29                     ` David Laight
2018-11-29 10:09                       ` Tetsuo Handa
2018-11-30 16:01                         ` Petr Mladek
2018-11-10  8:52               ` Tetsuo Handa
2018-11-23 12:52                 ` Petr Mladek
2018-11-09 14:08           ` Linus Torvalds
2018-11-09 14:42             ` Steven Rostedt
2018-11-08 11:53       ` Petr Mladek
2018-11-08 12:44         ` Sergey Senozhatsky
2018-11-08 14:21           ` Sergey Senozhatsky
2018-11-09  9:54     ` Tetsuo Handa
2018-11-02 14:40 ` [PATCH v6 1/3] printk: Add line-buffered printk() API Matthew Wilcox
2018-11-03  1:55   ` Tetsuo Handa
2018-11-02 18:12 ` kbuild test robot
2018-11-02 18:12   ` kbuild test robot
2018-11-06 14:35 ` Sergey Senozhatsky
2018-11-07 10:21   ` Petr Mladek
2018-11-07 12:54     ` Tetsuo Handa
2018-11-08  2:21     ` Sergey Senozhatsky
2018-11-08 11:24       ` Petr Mladek
2018-11-08 11:46         ` Tetsuo Handa
2018-11-08 12:30         ` Sergey Senozhatsky
2018-11-09 14:10           ` Petr Mladek
2018-11-12  7:59             ` Sergey Senozhatsky
2018-11-12 10:42               ` Tetsuo Handa
2018-11-17 10:14               ` Tetsuo Handa
2018-11-07 10:52   ` Tetsuo Handa
2018-11-07 11:01     ` David Laight
2018-11-07 12:00       ` Petr Mladek
2018-11-07 11:45     ` Petr Mladek
2018-11-08  2:30     ` Sergey Senozhatsky
2018-11-07 13:41 ` Petr Mladek
2018-11-07 14:06 ` Petr Mladek

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=deb8d78b-0593-2b8e-1c7a-9203aa77005f@i-love.sakura.ne.jp \
    --to=penguin-kernel@i-love.sakura.ne.jp \
    --cc=akpm@linux-foundation.org \
    --cc=dvyukov@google.com \
    --cc=fengguang.wu@intel.com \
    --cc=glider@google.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=torvalds@linux-foundation.org \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH 3/3] lockdep: Use line-buffered printk() for lockdep messages.' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.