| Message ID | 20221107141638.3790965-1-john.ogness@linutronix.de |
|---|---|
| Headers | show |
| Series | reduce console_lock scope | expand |
On Mon, Nov 07, 2022 at 03:22:00PM +0106, John Ogness wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Unprotected list walks are not necessarily safe. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: John Ogness <john.ogness@linutronix.de> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Reviewed-by: Petr Mladek <pmladek@suse.com> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> Daniel.
On Mon, Nov 07, 2022 at 03:22:34PM +0106, John Ogness wrote: > configure_kgdboc() uses the console_lock for console list iteration. Use > the console_list_lock instead because list synchronization responsibility > will be removed from the console_lock in a later change. > > The SRCU iterator could have been used here, but a later change will > relocate the locking of the console_list_lock to also provide > synchronization against register_console(). > > Note, the console_lock is still needed to serialize the device() > callback with other console operations. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > drivers/tty/serial/kgdboc.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > index 5be381003e58..82b4b4d67823 100644 > --- a/drivers/tty/serial/kgdboc.c > +++ b/drivers/tty/serial/kgdboc.c > @@ -451,6 +463,7 @@ static void kgdboc_earlycon_pre_exp_handler(void) > { > struct console *con; > static bool already_warned; > + int cookie; > > if (already_warned) > return; > @@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void) > * serial drivers might be OK with this, print a warning once per > * boot if we detect this case. > */ > - for_each_console(con) > + cookie = console_srcu_read_lock(); > + for_each_console_srcu(con) { > if (con == kgdboc_earlycon_io_ops.cons) > - return; > + break; > + } > + console_srcu_read_unlock(cookie); > + if (con) > + return; This change isn't mentioned in the patch description. Daniel.
On 2022-11-09, Daniel Thompson <daniel.thompson@linaro.org> wrote: >> @@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void) >> * serial drivers might be OK with this, print a warning once per >> * boot if we detect this case. >> */ >> - for_each_console(con) >> + cookie = console_srcu_read_lock(); >> + for_each_console_srcu(con) { >> if (con == kgdboc_earlycon_io_ops.cons) >> - return; >> + break; >> + } >> + console_srcu_read_unlock(cookie); >> + if (con) >> + return; > > This change isn't mentioned in the patch description. I will move this change into its own separate patch. tty: serial: kgdboc: use srcu console list iterator Use srcu console list iteration for safe console list traversal. Thanks. John
On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote: >>> -static inline bool uart_console_enabled(struct uart_port *port) >>> +/* Variant of uart_console_registered() when the console_list_lock is held. */ >>> +static inline bool uart_console_registered_locked(struct uart_port *port) >>> { >>> - return uart_console(port) && (port->cons->flags & CON_ENABLED); >>> + return uart_console(port) && console_is_registered_locked(port->cons); >>> +} >>> + >>> +static inline bool uart_console_registered(struct uart_port *port) >>> +{ >>> + bool ret; >>> + >>> + console_list_lock(); >>> + ret = uart_console_registered_locked(port); >>> + console_list_unlock(); >>> + return ret; >> >> Perhaps >> >> return uart_console(port) && console_is_registered(); >> >> to avoid locking the list when the first condition is not true? > > I do not have strong opinion on this. It is true that the code > duplication is trivial but it is a code duplication. Either > way would work for me. I will go with Geert's suggestion for v4. It is important that we reduce lock contention for non-console ports. > The reset of the code looks good. Feel free to use: > > Reviewed-by: Petr Mladek <pmladek@suse.com> Thanks. John
On Mon, Nov 07, 2022 at 03:22:36PM +0106, John Ogness wrote: > kgdboc_earlycon_init() uses the console_lock to ensure that no consoles > are unregistered until the kgdboc_earlycon is setup. The console_list_lock > should be used instead because list synchronization responsibility will > be removed from the console_lock in a later change. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> I've not looked at the other patches in the series to understand the future tense here (e.g. why we need intermediate patches like this one). However I've no objections to the change so: Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> Daniel. > --- > drivers/tty/serial/kgdboc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > index 8c2b7ccdfebf..a3ed9b34e2ab 100644 > --- a/drivers/tty/serial/kgdboc.c > +++ b/drivers/tty/serial/kgdboc.c > @@ -558,13 +558,13 @@ static int __init kgdboc_earlycon_init(char *opt) > */ > > /* > - * Hold the console_lock to guarantee that no consoles are > + * Hold the console_list_lock to guarantee that no consoles are > * unregistered until the kgdboc_earlycon setup is complete. > * Trapping the exit() callback relies on exit() not being > * called until the trap is setup. This also allows safe > * traversal of the console list and race-free reading of @flags. > */ > - console_lock(); > + console_list_lock(); > for_each_console(con) { > if (con->write && con->read && > (con->flags & (CON_BOOT | CON_ENABLED)) && > @@ -606,7 +606,7 @@ static int __init kgdboc_earlycon_init(char *opt) > } > > unlock: > - console_unlock(); > + console_list_unlock(); > > /* Non-zero means malformed option so we always return zero */ > return 0; > -- > 2.30.2 >
On Wed 2022-11-09 10:50:43, John Ogness wrote: > On 2022-11-09, Daniel Thompson <daniel.thompson@linaro.org> wrote: > >> @@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void) > >> * serial drivers might be OK with this, print a warning once per > >> * boot if we detect this case. > >> */ > >> - for_each_console(con) > >> + cookie = console_srcu_read_lock(); > >> + for_each_console_srcu(con) { > >> if (con == kgdboc_earlycon_io_ops.cons) > >> - return; > >> + break; > >> + } > >> + console_srcu_read_unlock(cookie); > >> + if (con) > >> + return; > > > > This change isn't mentioned in the patch description. > > I will move this change into its own separate patch. > > tty: serial: kgdboc: use srcu console list iterator > > Use srcu console list iteration for safe console list traversal. Yes, split it please :-) Anyway, both changes look good to me. Best Regards, Petr
On Mon 2022-11-07 15:22:36, John Ogness wrote: > kgdboc_earlycon_init() uses the console_lock to ensure that no consoles > are unregistered until the kgdboc_earlycon is setup. The console_list_lock > should be used instead because list synchronization responsibility will > be removed from the console_lock in a later change. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On 2022-11-07 09:15, John Ogness wrote: [...] > > The base commit for this series is from Paul McKenney's RCU tree > and provides an NMI-safe SRCU implementation [1]. Without the > NMI-safe SRCU implementation, this series is not less safe than > mainline. But we will need the NMI-safe SRCU implementation for > atomic consoles anyway, so we might as well get it in > now. Especially since it _does_ increase the reliability for > mainline in the panic path. So, your email got me to review the SRCU nmi-safe series: [1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a Especially this commit: https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=srcunmisafe.2022.10.21a&id=5d0f5953b60f5f7a278085b55ddc73e2932f4c33 I disagree with the overall approach taken there, which is to create yet another SRCU flavor, this time with explicit "nmi-safe" read-locks. This adds complexity to the kernel APIs and I think we can be clever about this and make SRCU nmi-safe without requiring a whole new incompatible API. You can find the basic idea needed to achieve this in the libside RCU user-space implementation. I needed to introduce a split-counter concept to support rseq vs atomics to keep track of per-cpu grace period counters. The "rseq" counter is the fast-path, but if rseq fails, the abort handler uses the atomic counter instead. https://github.com/compudj/side/blob/main/src/rcu.h#L23 struct side_rcu_percpu_count { uintptr_t begin; uintptr_t rseq_begin; uintptr_t end; uintptr_t rseq_end; } __attribute__((__aligned__(SIDE_CACHE_LINE_SIZE))); The idea is to "split" each percpu counter into two counters, one for rseq, and the other for atomics. When a grace period wants to observe the value of a percpu counter, it simply sums the two counters: https://github.com/compudj/side/blob/main/src/rcu.c#L112 The same idea can be applied to SRCU in the kernel: one counter for percpu ops, and the other counter for nmi context, so basically: srcu_read_lock() if (likely(!in_nmi())) increment the percpu-ops lock counter else increment the atomic lock counter srcu_read_unlock() if (likely(!in_nmi())) increment the percpu-ops unlock counter else increment the atomic unlock counter Then in the grace period sum the percpu-ops and the atomic values whenever each counter value is read. This would allow SRCU to be NMI-safe without requiring the callers to explicitly state whether they need to be nmi-safe or not, and would only take the overhead of the atomics in the NMI handlers rather than for all users which happen to use SRCU read locks shared with nmi handlers. Thoughts ? Thanks, Mathieu
This is v3 of a series to prepare for threaded/atomic printing. v2 is here [0]. This series focuses on reducing the scope of the BKL console_lock. It achieves this by switching to SRCU and a dedicated mutex for console list iteration and modification, respectively. The console_lock will no longer offer this protection and is completely removed from (un)register_console() and console_stop/start() code. Also, during the review of v2 it came to our attention that many console drivers are checking CON_ENABLED to see if they are registered. Because this flag can change without unregistering and because this flag does not represent an atomic point when an (un)registration process is complete, a new console_is_registered() function is introduced. This function uses the console_list_lock to synchronize with the (un)registration process to provide a reliable status. All users of the console_lock for list iteration have been modified. For the call sites where the console_lock is still needed (because of other reasons), comments are added to explain exactly why the console_lock was needed. All users of CON_ENABLED for registration status have been modified to use console_is_registered(). Note that there are still users of CON_ENABLED, but this is for legitimate purposes about a registered console being able to print. The base commit for this series is from Paul McKenney's RCU tree and provides an NMI-safe SRCU implementation [1]. Without the NMI-safe SRCU implementation, this series is not less safe than mainline. But we will need the NMI-safe SRCU implementation for atomic consoles anyway, so we might as well get it in now. Especially since it _does_ increase the reliability for mainline in the panic path. Changes since v3: general: - introduce a synchronized console_is_registered() to query if a console is registered, meant to replace CON_ENABLED (mis)use for this purpose - directly read console->flags for registered consoles if it is race-free (and document that it is so) - replace uart_console_enabled() with a new uart_console_registered() based on console_is_registered() - change comments about why console_lock is used to synchronize console->device() by providing an example registration check fixups: - the following drivers were modified to use the new console_is_registered() instead of CON_ENABLED checks - arch/m68k/emu/nfcon.c - drivers/firmware/efi/earlycon.c - drivers/net/netconsole.c - drivers/tty/hvc/hvc_console.c - drivers/tty/serial/8250/8250_core.c - drivers/tty/serial/earlycon.c - drivers/tty/serial/pic32_uart.c - drivers/tty/serial/samsung_tty.c - drivers/tty/serial/serial_core.c - drivers/tty/serial/xilinx_uartps.c - drivers/usb/early/xhci-dbc.c um: kmsg_dumper: - change stdout dump criteria to match original intention kgdb/kdb: - in configure_kgdboc(), take console_list_lock to synchronize tty_find_polling_driver() against register_console() - add comments explaining why calling console->write() without locking might work tty: sh-sci: - use a setup() callback to setup the early console fbdev: xen: - implement a cleaner approach for console_force_preferred_locked() rcu: - implement debug_lockdep_rcu_enabled() for !CONFIG_DEBUG_LOCK_ALLOC printk: - check CONFIG_DEBUG_LOCK_ALLOC for srcu_read_lock_held() availability - for console_lock/_trylock/_unlock, replace "lock the console system" language with "block the console subsystem from printing" - use WRITE_ONCE() for updating console->flags of registered consoles - expand comments of synchronize_srcu() calls to explain why they are needed, and also expand comments to explain when it is not needed - change CON_BOOT consoles to always begin at earliest message - for non-BOOT/non-PRINTBUFFER consoles, initialize @seq to the minimal @seq of any of the enabled boot consoles - add comments and lockdep assertion to unregister_console_locked() because it is not clear from the name which lock is implied - dropped patches that caused unnecessary churn in the series John Ogness [0] https://lore.kernel.org/lkml/20221019145600.1282823-1-john.ogness@linutronix.de [1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a John Ogness (38): rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC printk: Prepare for SRCU console list protection printk: fix setting first seq for consoles um: kmsg_dump: only dump when no output console available console: introduce console_is_enabled() wrapper printk: use console_is_enabled() um: kmsg_dump: use console_is_enabled() kdb: kdb_io: use console_is_enabled() um: kmsg_dumper: use srcu console list iterator tty: serial: kgdboc: document console_lock usage tty: tty_io: document console_lock usage proc: consoles: document console_lock usage kdb: use srcu console list iterator printk: console_flush_all: use srcu console list iterator printk: console_unblank: use srcu console list iterator printk: console_flush_on_panic: use srcu console list iterator printk: console_device: use srcu console list iterator printk: __pr_flush: use srcu console list iterator printk: introduce console_list_lock console: introduce console_is_registered() serial_core: replace uart_console_enabled() with uart_console_registered() tty: nfcon: use console_is_registered() efi: earlycon: use console_is_registered() tty: hvc: use console_is_registered() tty: serial: earlycon: use console_is_registered() tty: serial: pic32_uart: use console_is_registered() tty: serial: samsung_tty: use console_is_registered() tty: serial: xilinx_uartps: use console_is_registered() usb: early: xhci-dbc: use console_is_registered() netconsole: avoid CON_ENABLED misuse to track registration printk, xen: fbfront: create/use safe function for forcing preferred tty: tty_io: use console_list_lock for list synchronization proc: consoles: use console_list_lock for list iteration tty: serial: kgdboc: use console_list_lock for list traversal tty: serial: kgdboc: synchronize tty_find_polling_driver() and register_console() tty: serial: kgdboc: use console_list_lock to trap exit printk: relieve console_lock of list synchronization duties tty: serial: sh-sci: use setup() callback for early console Thomas Gleixner (2): serial: kgdboc: Lock console list in probe function printk: Convert console_drivers list to hlist .clang-format | 1 + arch/m68k/emu/nfcon.c | 10 +- arch/um/kernel/kmsg_dump.c | 24 +- drivers/firmware/efi/earlycon.c | 8 +- drivers/net/netconsole.c | 21 +- drivers/tty/hvc/hvc_console.c | 4 +- drivers/tty/serial/8250/8250_core.c | 2 +- drivers/tty/serial/earlycon.c | 4 +- drivers/tty/serial/kgdboc.c | 46 ++- drivers/tty/serial/pic32_uart.c | 4 +- drivers/tty/serial/samsung_tty.c | 2 +- drivers/tty/serial/serial_core.c | 14 +- drivers/tty/serial/sh-sci.c | 17 +- drivers/tty/serial/xilinx_uartps.c | 2 +- drivers/tty/tty_io.c | 18 +- drivers/usb/early/xhci-dbc.c | 2 +- drivers/video/fbdev/xen-fbfront.c | 12 +- fs/proc/consoles.c | 21 +- include/linux/console.h | 111 +++++++- include/linux/rcupdate.h | 5 + include/linux/serial_core.h | 15 +- kernel/debug/kdb/kdb_io.c | 14 +- kernel/printk/printk.c | 424 +++++++++++++++++++++------- 23 files changed, 605 insertions(+), 176 deletions(-) base-commit: e29a4915db1480f96e0bc2e928699d086a71f43c