lkml.org 
[lkml]   [2022]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch RFC 00/29] printk: A new approach - WIP
On Sun, Sep 11, 2022 at 12:27:31AM +0200, Thomas Gleixner wrote:

[ . . . ]

> The infrastructure we implemented comes with the following properties:
>
> - It shares the console list, but only for registration/unregistration
> purposes. Consoles which are utilizing the new infrastructure are
> ignored by the existing mechanisms and vice versa. This allows to
> reuse as much code as possible and preserves the printk semantics
> except for the threaded printing part.
>
> - The console list walk becomes SRCU protected to avoid any restrictions
> on contexts

I am guessing that this means that you need an NMI-safe srcu_read_lock()
and srcu_read_unlock(). If my guess is correct, please let me know,
and I will create one for you. (As it stands, these are NMI-safe on x86,
but not on architectures using the asm-generic variant of this_cpu_inc().

The result would be srcu_read_lock_nmi() and srcu_read_unlock_nmi()
or similar. There would need to be something to prevent mixing of
srcu_read_lock() and srcu_read_lock_nmi().

Or are you somehow avoiding ever invoking either srcu_read_lock() or
srcu_read_unlock() from NMI context?

For example, are we simply living dangerously with NMI-based stack traces
as suggested below? ;-)

> - Consoles become stateful to handle handover and takeover in a graceful
> way.
>
> - All console state operations rely solely on atomic*_try_cmpxchg() so
> they work in any context.
>
> - Console locking is per console to allow friendly handover or "safe"
> hostile takeover in emergency/panic situations. Console lock is not
> relevant for consoles converted to the new infrastructure.
>
> - The core provides interfaces for console drivers to query whether they
> can proceed and to denote 'unsafe' sections in the console state, which
> is unavoidable for some console drivers.
>
> In fact there is not a single regular (non-early) console driver today
> which is reentrancy safe under all circumstances, which enforces that
> NMI context is excluded from printing directly. TBH, that's a sad state
> of affairs.
>
> The unsafe state bit allows to make informed decisions in the core
> code, e.g. to postpone printing if there are consoles available which
> are safe to acquire. In case of a hostile takeover the unsafe state bit
> is handed to the atomic write callback so that the console driver can
> act accordingly.
>
> - Printing is delegated to a per console printer thread except for the
> following situations:
>
> - Early boot
> - Emergency printing (WARN/OOPS)
> - Panic printing
>
> The integration is complete, but without any fancy things, like locking all
> consoles when entering a WARN, print and unlock when done. Such things only
> make sense once all drivers are converted over because that conflicts with
> the way how the existing console lock mechanics work.
>
> For testing we used the most simple driver: a hacked up version of the
> early uart8250 console as we wanted to concentrate on validating the core
> mechanisms of friendly handover and hostile takeovers instead of dealing
> with the horrors of port locks or whatever at the same time. That's the
> next challenge. Hack patch will be provided in a reply.
>
> Here is sample output where we let the atomic and thread write functions
> prepend each line with the printing context (A=atomic, T=thread):
>
> A[ 0.394066] ... fixed-purpose events: 3
> A[ 0.395130] ... event mask: 000000070000000f
>
> End of early boot, thread starts
>
> TA[ 0.396821] rcu: Hierarchical SRCU implementation.
>
> ^^ Thread starts printing and immediately raises a warning, so atomic
> context at the emergency priority takes over and continues printing.
>
> This is a forceful, but safe takeover scenario as the WARN context
> is obviously on the same CPU as the printing thread where friendly
> is not an option.
>
> A[ 0.397133] rcu: Max phase no-delay instances is 400.
> A[ 0.397640] ------------[ cut here ]------------
> A[ 0.398072] WARNING: CPU: 0 PID: 13 at drivers/tty/serial/8250/8250_early.c:123 __early_serial8250_write.isra.0+0x80/0xa0
> ....
> A[ 0.440131] ret_from_fork+0x1f/0x30
> A[ 0.441133] </TASK>
> A[ 0.441867] ---[ end trace 0000000000000000 ]---
> T[ 0.443493] smp: Bringing up secondary CPUs ...
>
> After the warning the thread continues printing.
>
> ....
>
> T[ 1.916873] input: ImExPS/2 Generic Explorer Mouse as /devices/platform/i8042/serio1/input/input3
> T[ 1.918719] md: Waiting for all devices to be available before autod
>
> A[ 1.918719] md: Waiting for all devices to be available before autodetect
>
> System panics because it can't find a root file system. Panic printing
> takes over the console from the printer thread immediately and reprints
> the interrupted line.
>
> This case is a friendly handover from the printing thread to the panic
> context because the printing thread was not running on the panic CPU, but
> handed over gracefully.
>
> A[ 1.919942] md: If you don't use raid, use raid=noautodetect
> A[ 1.921030] md: Autodetecting RAID arrays.
> A[ 1.921919] md: autorun ...
> A[ 1.922686] md: ... autorun DONE.
> A[ 1.923761] /dev/root: Can't open blockdev
>
> So far the implemented state handling machinery holds up on the various
> handover and hostile takeover situations we enforced for testing.
>
> Hostile takeover is nevertheless a special case. If the driver is in an
> unsafe region that's something which needs to be dealt with per driver.
> There is not much the core code can do here except of trying a friendly
> handover first and only enforcing it after a timeout or not trying to print
> on such consoles.
>
> This needs some thought, but we explicitely did not implement any takeover
> policy into the core state handling mechanism as this is really a decision
> which needs to be made at the call site. See patch 28.
>
> We are soliciting feedback on that approach and we hope that we can
> organize a BOF in Dublin on short notice.

Last I checked, there were still some slots open.

> The series is also available from git:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git printk
>
> The series has the following parts:
>
> Patches 1 - 5: Cleanups
>
> Patches 6 - 12: Locking and list conversion
>
> Patches 13 - 18: Improved output buffer handling to prepare for
> code sharing
>
> Patches 19 - 29: New infrastructure implementation
>
> Most of the preparatory patches 1-18 have probably a value on their own.
>
> Don't be scared about the patch stats below. We added kernel doc and
> extensive comments to the code:
>
> kernel/printk/printk_nobkl.c: Code: 668 lines, Comments: 697 lines, Ratio: 1:1.043

When I reach stable AC, I will fire off some rcutorture tests. From the
discussion above, it sounds like I should expect some boot-time warnings?
Or were those strictly printk-specific testing?

> Of course the code is trivial and straight forward as any other facility
> which has to deal with concurrency and the twist of being safe in any
> context. :)

;-) ;-) ;-)

Thanx, Paul

> Comments welcome.
>
> Thanks,
>
> tglx
> ---
> [1] https://lore.kernel.org/lkml/87r11qp63n.ffs@tglx/
> ---
> arch/parisc/include/asm/pdc.h | 2
> arch/parisc/kernel/pdc_cons.c | 53 -
> arch/parisc/kernel/traps.c | 17
> b/kernel/printk/printk_nobkl.c | 1564 +++++++++++++++++++++++++++++++++++++++++
> drivers/tty/serial/kgdboc.c | 7
> drivers/tty/tty_io.c | 6
> fs/proc/consoles.c | 12
> fs/proc/kmsg.c | 2
> include/linux/console.h | 375 +++++++++
> include/linux/printk.h | 9
> include/linux/syslog.h | 3
> kernel/debug/kdb/kdb_io.c | 7
> kernel/panic.c | 12
> kernel/printk/printk.c | 485 ++++++++----
> 14 files changed, 2304 insertions(+), 250 deletions(-)

\
 
 \ /
  Last update: 2022-09-11 11:03    [W:0.283 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site