Hi,
This is v2 of a series working towards threaded/atomic console
printing. This series is only a subset of the original
v1 [0]. That series began with various cleanups before adding
the threaded/atomic code. Since none of those cleanups made it
into the recent 6.5 merge window, I have decided to post them
as their own series. I hope this helps to get them accepted
without being attached to any threaded/atomic discussions.
Changes since v1:
- drop patches 5-18 (they will return in a follow-up series)
- console_unblank() aborts if called from NMI context
- console_flush_on_panic() directly flushes rather than using
console_lock/_unlock dance (because console_lock/_unlock is not
NMI-safe)
- remove @console_suspended and rely only on CON_SUSPENDED flag
- rename abandon_console_lock_in_panic() to
other_cpu_in_panic()
- console_trylock() and console_lock() will fail and block,
respectively, while another CPU is in panic
- adjust various comments and whitespace as suggested
John Ogness
[0] https://lore.kernel.org/lkml/[email protected]
John Ogness (5):
kdb: do not assume write() callback available
printk: Add NMI safety to console_flush_on_panic() and
console_unblank()
printk: Consolidate console deferred printing
printk: Add per-console suspended state
printk: Rename abandon_console_lock_in_panic() to other_cpu_in_panic()
include/linux/console.h | 3 +
kernel/debug/kdb/kdb_io.c | 2 +
kernel/printk/internal.h | 2 +
kernel/printk/printk.c | 189 +++++++++++++++++++++++-------------
kernel/printk/printk_safe.c | 9 +-
5 files changed, 132 insertions(+), 73 deletions(-)
base-commit: 7ec85f3e089aa423a69559bf4555b6218b5a2ef7
--
2.30.2
It is allowed for consoles to provide no write() callback. For
example ttynull does this.
Check if a write() callback is available before using it.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 5c7e9ba7cd6b..e9139dfc1f0a 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -576,6 +576,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
continue;
if (c == dbg_io_ops->cons)
continue;
+ if (!c->write)
+ continue;
/*
* Set oops_in_progress to encourage the console drivers to
* disregard their internal spin locks: in the current calling
--
2.30.2
Currently abandon_console_lock_in_panic() is only used to determine if
the current CPU should immediately release the console lock because
another CPU is in panic. However, later this function will be used by
the CPU to immediately release other resources in this situation.
Rename the function to other_cpu_in_panic(), which is a better
description and does not assume it is related to the console lock.
Signed-off-by: John Ogness <[email protected]>
---
kernel/printk/internal.h | 2 ++
kernel/printk/printk.c | 15 ++++++++-------
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 2a17704136f1..7d4979d5c3ce 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -103,3 +103,5 @@ struct printk_message {
u64 seq;
unsigned long dropped;
};
+
+bool other_cpu_in_panic(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 701908d389f4..529671aaec2d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2612,11 +2612,12 @@ static int console_cpu_notify(unsigned int cpu)
}
/*
- * Return true when this CPU should unlock console_sem without pushing all
- * messages to the console. This reduces the chance that the console is
- * locked when the panic CPU tries to use it.
+ * Return true if a panic is in progress on a remote CPU.
+ *
+ * On true, the local CPU should immediately release any printing resources
+ * that may be needed by the panic CPU.
*/
-static bool abandon_console_lock_in_panic(void)
+bool other_cpu_in_panic(void)
{
if (!panic_in_progress())
return false;
@@ -2643,7 +2644,7 @@ void console_lock(void)
might_sleep();
/* On panic, the console_lock must be left to the panic cpu. */
- while (abandon_console_lock_in_panic())
+ while (other_cpu_in_panic())
msleep(1000);
down_console_sem();
@@ -2663,7 +2664,7 @@ EXPORT_SYMBOL(console_lock);
int console_trylock(void)
{
/* On panic, the console_lock must be left to the panic cpu. */
- if (abandon_console_lock_in_panic())
+ if (other_cpu_in_panic())
return 0;
if (down_trylock_console_sem())
return 0;
@@ -2978,7 +2979,7 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
any_progress = true;
/* Allow panic_cpu to take over the consoles safely. */
- if (abandon_console_lock_in_panic())
+ if (other_cpu_in_panic())
goto abandon;
if (do_cond_resched)
--
2.30.2
The printk path is NMI safe because it only adds content to the
buffer and then triggers the delayed output via irq_work. If the
console is flushed or unblanked on panic (from NMI context) then it
can deadlock in down_trylock_console_sem() because the semaphore is
not NMI safe.
Avoid taking the console lock when flushing in panic. To prevent
other CPUs from taking the console lock while flushing, have
console_lock() block and console_trylock() fail for non-panic CPUs
during panic.
Skip unblanking in panic if the current context is NMI.
Signed-off-by: John Ogness <[email protected]>
---
kernel/printk/printk.c | 77 +++++++++++++++++++++++++++---------------
1 file changed, 49 insertions(+), 28 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9644f6e5bf15..8a6c917dc081 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2583,6 +2583,25 @@ static int console_cpu_notify(unsigned int cpu)
return 0;
}
+/*
+ * Return true when this CPU should unlock console_sem without pushing all
+ * messages to the console. This reduces the chance that the console is
+ * locked when the panic CPU tries to use it.
+ */
+static bool abandon_console_lock_in_panic(void)
+{
+ if (!panic_in_progress())
+ return false;
+
+ /*
+ * We can use raw_smp_processor_id() here because it is impossible for
+ * the task to be migrated to the panic_cpu, or away from it. If
+ * panic_cpu has already been set, and we're not currently executing on
+ * that CPU, then we never will be.
+ */
+ return atomic_read(&panic_cpu) != raw_smp_processor_id();
+}
+
/**
* console_lock - block the console subsystem from printing
*
@@ -2595,6 +2614,10 @@ void console_lock(void)
{
might_sleep();
+ /* On panic, the console_lock must be left to the panic cpu. */
+ while (abandon_console_lock_in_panic())
+ msleep(1000);
+
down_console_sem();
if (console_suspended)
return;
@@ -2613,6 +2636,9 @@ EXPORT_SYMBOL(console_lock);
*/
int console_trylock(void)
{
+ /* On panic, the console_lock must be left to the panic cpu. */
+ if (abandon_console_lock_in_panic())
+ return 0;
if (down_trylock_console_sem())
return 0;
if (console_suspended) {
@@ -2631,25 +2657,6 @@ int is_console_locked(void)
}
EXPORT_SYMBOL(is_console_locked);
-/*
- * Return true when this CPU should unlock console_sem without pushing all
- * messages to the console. This reduces the chance that the console is
- * locked when the panic CPU tries to use it.
- */
-static bool abandon_console_lock_in_panic(void)
-{
- if (!panic_in_progress())
- return false;
-
- /*
- * We can use raw_smp_processor_id() here because it is impossible for
- * the task to be migrated to the panic_cpu, or away from it. If
- * panic_cpu has already been set, and we're not currently executing on
- * that CPU, then we never will be.
- */
- return atomic_read(&panic_cpu) != raw_smp_processor_id();
-}
-
/*
* Check if the given console is currently capable and allowed to print
* records.
@@ -3054,6 +3061,10 @@ void console_unblank(void)
* In that case, attempt a trylock as best-effort.
*/
if (oops_in_progress) {
+ /* Semaphores are not NMI-safe. */
+ if (in_nmi())
+ return;
+
if (down_trylock_console_sem() != 0)
return;
} else
@@ -3083,14 +3094,24 @@ void console_unblank(void)
*/
void console_flush_on_panic(enum con_flush_mode mode)
{
+ bool handover;
+ u64 next_seq;
+
/*
- * If someone else is holding the console lock, trylock will fail
- * and may_schedule may be set. Ignore and proceed to unlock so
- * that messages are flushed out. As this can be called from any
- * context and we don't want to get preempted while flushing,
- * ensure may_schedule is cleared.
+ * Ignore the console lock and flush out the messages. Attempting a
+ * trylock would not be useful because:
+ *
+ * - if it is contended, it must be ignored anyway
+ * - console_lock() and console_trylock() block and fail
+ * respectively in panic for non-panic CPUs
+ * - semaphores are not NMI-safe
+ */
+
+ /*
+ * If another context is holding the console lock,
+ * @console_may_schedule might be set. Clear it so that
+ * this context does not call cond_resched() while flushing.
*/
- console_trylock();
console_may_schedule = 0;
if (mode == CONSOLE_REPLAY_ALL) {
@@ -3103,15 +3124,15 @@ void console_flush_on_panic(enum con_flush_mode mode)
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
/*
- * If the above console_trylock() failed, this is an
- * unsynchronized assignment. But in that case, the
+ * This is an unsynchronized assignment, but the
* kernel is in "hope and pray" mode anyway.
*/
c->seq = seq;
}
console_srcu_read_unlock(cookie);
}
- console_unlock();
+
+ console_flush_all(false, &next_seq, &handover);
}
/*
--
2.30.2
Printing to consoles can be deferred for several reasons:
- explicitly with printk_deferred()
- printk() in NMI context
- recursive printk() calls
The current implementation is not consistent. For printk_deferred(),
irq work is scheduled twice. For NMI und recursive, panic CPU
suppression and caller delays are not properly enforced.
Correct these inconsistencies by consolidating the deferred printing
code so that vprintk_deferred() is the top-level function for
deferred printing and vprintk_emit() will perform whichever irq_work
queueing is appropriate.
Also add kerneldoc for wake_up_klogd() and defer_console_output() to
clarify their differences and appropriate usage.
Signed-off-by: John Ogness <[email protected]>
---
kernel/printk/printk.c | 35 ++++++++++++++++++++++++++++-------
kernel/printk/printk_safe.c | 9 ++-------
2 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8a6c917dc081..e7632ed3b6fd 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2306,7 +2306,11 @@ asmlinkage int vprintk_emit(int facility, int level,
preempt_enable();
}
- wake_up_klogd();
+ if (in_sched)
+ defer_console_output();
+ else
+ wake_up_klogd();
+
return printed_len;
}
EXPORT_SYMBOL(vprintk_emit);
@@ -3817,11 +3821,33 @@ static void __wake_up_klogd(int val)
preempt_enable();
}
+/**
+ * wake_up_klogd - Wake kernel logging daemon
+ *
+ * Use this function when new records have been added to the ringbuffer
+ * and the console printing of those records has already occurred or is
+ * known to be handled by some other context. This function will only
+ * wake the logging daemon.
+ *
+ * Context: Any context.
+ */
void wake_up_klogd(void)
{
__wake_up_klogd(PRINTK_PENDING_WAKEUP);
}
+/**
+ * defer_console_output - Wake kernel logging daemon and trigger
+ * console printing in a deferred context
+ *
+ * Use this function when new records have been added to the ringbuffer,
+ * this context is responsible for console printing those records, but
+ * the current context is not allowed to perform the console printing.
+ * Trigger an irq_work context to perform the console printing. This
+ * function also wakes the logging daemon.
+ *
+ * Context: Any context.
+ */
void defer_console_output(void)
{
/*
@@ -3838,12 +3864,7 @@ void printk_trigger_flush(void)
int vprintk_deferred(const char *fmt, va_list args)
{
- int r;
-
- r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
- defer_console_output();
-
- return r;
+ return vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
}
int _printk_deferred(const char *fmt, ...)
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index ef0f9a2044da..6d10927a07d8 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -38,13 +38,8 @@ asmlinkage int vprintk(const char *fmt, va_list args)
* Use the main logbuf even in NMI. But avoid calling console
* drivers that might have their own locks.
*/
- if (this_cpu_read(printk_context) || in_nmi()) {
- int len;
-
- len = vprintk_store(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
- defer_console_output();
- return len;
- }
+ if (this_cpu_read(printk_context) || in_nmi())
+ return vprintk_deferred(fmt, args);
/* No obstacles. */
return vprintk_default(fmt, args);
--
2.30.2
Currently the global @console_suspended is used to determine if
consoles are in a suspended state. Its primary purpose is to allow
usage of the console_lock when suspended without causing console
printing. It is synchronized by the console_lock.
Rather than relying on the console_lock to determine suspended
state, make it an official per-console state that is set within
console->flags. This allows the state to be queried via SRCU.
Remove @console_suspended. Console printing will still be avoided
when suspended because console_is_usable() returns false when
the new suspended flag is set for that console.
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 3 ++
kernel/printk/printk.c | 74 ++++++++++++++++++++++++-----------------
2 files changed, 47 insertions(+), 30 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index d3195664baa5..7de11c763eb3 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -154,6 +154,8 @@ static inline int con_debug_leave(void)
* receiving the printk spam for obvious reasons.
* @CON_EXTENDED: The console supports the extended output format of
* /dev/kmesg which requires a larger output buffer.
+ * @CON_SUSPENDED: Indicates if a console is suspended. If true, the
+ * printing callbacks must not be called.
*/
enum cons_flags {
CON_PRINTBUFFER = BIT(0),
@@ -163,6 +165,7 @@ enum cons_flags {
CON_ANYTIME = BIT(4),
CON_BRL = BIT(5),
CON_EXTENDED = BIT(6),
+ CON_SUSPENDED = BIT(7),
};
/**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e7632ed3b6fd..701908d389f4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -86,7 +86,7 @@ EXPORT_SYMBOL(oops_in_progress);
static DEFINE_MUTEX(console_mutex);
/*
- * console_sem protects updates to console->seq and console_suspended,
+ * console_sem protects updates to console->seq
* and also provides serialization for console printing.
*/
static DEFINE_SEMAPHORE(console_sem);
@@ -359,7 +359,7 @@ static bool panic_in_progress(void)
* paths in the console code where we end up in places I want
* locked without the console semaphore held).
*/
-static int console_locked, console_suspended;
+static int console_locked;
/*
* Array of consoles built from command line options (console=)
@@ -2549,22 +2549,46 @@ MODULE_PARM_DESC(console_no_auto_verbose, "Disable console loglevel raise to hig
*/
void suspend_console(void)
{
+ struct console *con;
+
if (!console_suspend_enabled)
return;
pr_info("Suspending console(s) (use no_console_suspend to debug)\n");
pr_flush(1000, true);
- console_lock();
- console_suspended = 1;
- up_console_sem();
+
+ console_list_lock();
+ for_each_console(con)
+ console_srcu_write_flags(con, con->flags | CON_SUSPENDED);
+ console_list_unlock();
+
+ /*
+ * Ensure that all SRCU list walks have completed. All printing
+ * contexts must be able to see that they are suspended so that it
+ * is guaranteed that all printing has stopped when this function
+ * completes.
+ */
+ synchronize_srcu(&console_srcu);
}
void resume_console(void)
{
+ struct console *con;
+
if (!console_suspend_enabled)
return;
- down_console_sem();
- console_suspended = 0;
- console_unlock();
+
+ console_list_lock();
+ for_each_console(con)
+ console_srcu_write_flags(con, con->flags & ~CON_SUSPENDED);
+ console_list_unlock();
+
+ /*
+ * Ensure that all SRCU list walks have completed. All printing
+ * contexts must be able to see they are no longer suspended so
+ * that they are guaranteed to wake up and resume printing.
+ */
+ synchronize_srcu(&console_srcu);
+
pr_flush(1000, true);
}
@@ -2623,8 +2647,6 @@ void console_lock(void)
msleep(1000);
down_console_sem();
- if (console_suspended)
- return;
console_locked = 1;
console_may_schedule = 1;
}
@@ -2645,10 +2667,6 @@ int console_trylock(void)
return 0;
if (down_trylock_console_sem())
return 0;
- if (console_suspended) {
- up_console_sem();
- return 0;
- }
console_locked = 1;
console_may_schedule = 0;
return 1;
@@ -2674,6 +2692,9 @@ static inline bool console_is_usable(struct console *con)
if (!(flags & CON_ENABLED))
return false;
+ if ((flags & CON_SUSPENDED))
+ return false;
+
if (!con->write)
return false;
@@ -2992,11 +3013,6 @@ void console_unlock(void)
bool flushed;
u64 next_seq;
- if (console_suspended) {
- up_console_sem();
- return;
- }
-
/*
* Console drivers are called with interrupts disabled, so
* @console_may_schedule should be cleared before; however, we may
@@ -3702,8 +3718,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
/*
* Hold the console_lock to guarantee safe access to
- * console->seq and to prevent changes to @console_suspended
- * until all consoles have been processed.
+ * console->seq.
*/
console_lock();
@@ -3711,6 +3726,11 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
for_each_console_srcu(c) {
if (con && con != c)
continue;
+ /*
+ * If consoles are not usable, it cannot be expected
+ * that they make forward progress, so only increment
+ * @diff for usable consoles.
+ */
if (!console_is_usable(c))
continue;
printk_seq = c->seq;
@@ -3719,18 +3739,12 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
}
console_srcu_read_unlock(cookie);
- /*
- * If consoles are suspended, it cannot be expected that they
- * make forward progress, so timeout immediately. @diff is
- * still used to return a valid flush status.
- */
- if (console_suspended)
- remaining = 0;
- else if (diff != last_diff && reset_on_progress)
+ if (diff != last_diff && reset_on_progress)
remaining = timeout_ms;
console_unlock();
+ /* Note: @diff is 0 if there are no usable consoles. */
if (diff == 0 || remaining == 0)
break;
@@ -3764,7 +3778,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
* printer has been seen to make some forward progress.
*
* Context: Process context. May sleep while acquiring console lock.
- * Return: true if all enabled printers are caught up.
+ * Return: true if all usable printers are caught up.
*/
static bool pr_flush(int timeout_ms, bool reset_on_progress)
{
--
2.30.2
On (23/07/10 15:51), John Ogness wrote:
> It is allowed for consoles to provide no write() callback. For
> example ttynull does this.
>
> Check if a write() callback is available before using it.
>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On (23/07/10 15:51), John Ogness wrote:
> Currently abandon_console_lock_in_panic() is only used to determine if
> the current CPU should immediately release the console lock because
> another CPU is in panic. However, later this function will be used by
> the CPU to immediately release other resources in this situation.
>
> Rename the function to other_cpu_in_panic(), which is a better
> description and does not assume it is related to the console lock.
>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On Mon, Jul 10, 2023 at 03:51:20PM +0206, John Ogness wrote:
> It is allowed for consoles to provide no write() callback. For
> example ttynull does this.
>
> Check if a write() callback is available before using it.
>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>
For v1 I shared an ack rather than queuing the patch. Although reading
the thread back it is possible that was based on a misunderstanding
(https://lore.kernel.org/lkml/[email protected]/ ).
Anyhow, it looks like you have designed the new series to be picked
individually?
Daniel.
On Tue, Jul 11, 2023 at 11:04:11AM +0206, John Ogness wrote:
> On 2023-07-11, Daniel Thompson <[email protected]> wrote:
> > For v1 I shared an ack rather than queuing the patch. Although reading
> > the thread back it is possible that was based on a misunderstanding
> > (https://lore.kernel.org/lkml/[email protected]/ ).
> >
> > Anyhow, it looks like you have designed the new series to be picked
> > individually?
>
> I understood that Petr will carry the patch. Yes, this series only
> includes the non-atomic/non-threaded cleanups so that it will be easier
> for Petr to send the full series off to linux-next.
No worries, that's fine for me and from my point-of-view its still:
Acked-by: Daniel Thompson <[email protected]>
> For this patch there is nothing left to do. I should have removed the
> kdb people/lists from the mailing. Sorry about that.
To be honest I'd rather be in the loop than out (and with that title my
mail filters would jump in it anyway).
Daniel.
On 2023-07-11, Daniel Thompson <[email protected]> wrote:
> For v1 I shared an ack rather than queuing the patch. Although reading
> the thread back it is possible that was based on a misunderstanding
> (https://lore.kernel.org/lkml/[email protected]/ ).
>
> Anyhow, it looks like you have designed the new series to be picked
> individually?
I understood that Petr will carry the patch. Yes, this series only
includes the non-atomic/non-threaded cleanups so that it will be easier
for Petr to send the full series off to linux-next.
For this patch there is nothing left to do. I should have removed the
kdb people/lists from the mailing. Sorry about that.
John Ogness
On (23/07/10 15:51), John Ogness wrote:
[..]
> @@ -2623,8 +2647,6 @@ void console_lock(void)
> msleep(1000);
>
> down_console_sem();
> - if (console_suspended)
> - return;
> console_locked = 1;
> console_may_schedule = 1;
> }
> @@ -2645,10 +2667,6 @@ int console_trylock(void)
> return 0;
> if (down_trylock_console_sem())
> return 0;
> - if (console_suspended) {
> - up_console_sem();
> - return 0;
> - }
> console_locked = 1;
> console_may_schedule = 0;
> return 1;
Interesting. console_locked previously would not be set if
console is suspended, but now it's always set, which in theory
changes the way WARN_CONSOLE_UNLOCKED() macro works in some
cases?
On (23/07/10 15:51), John Ogness wrote:
> Printing to consoles can be deferred for several reasons:
>
> - explicitly with printk_deferred()
> - printk() in NMI context
> - recursive printk() calls
>
> The current implementation is not consistent. For printk_deferred(),
> irq work is scheduled twice. For NMI und recursive, panic CPU
> suppression and caller delays are not properly enforced.
>
> Correct these inconsistencies by consolidating the deferred printing
> code so that vprintk_deferred() is the top-level function for
> deferred printing and vprintk_emit() will perform whichever irq_work
> queueing is appropriate.
>
> Also add kerneldoc for wake_up_klogd() and defer_console_output() to
> clarify their differences and appropriate usage.
>
> Signed-off-by: John Ogness <[email protected]>
Looks good to me
Reviewed-by: Sergey Senozhatsky <[email protected]>
On 2023-07-12, Sergey Senozhatsky <[email protected]> wrote:
>> @@ -2623,8 +2647,6 @@ void console_lock(void)
>> msleep(1000);
>>
>> down_console_sem();
>> - if (console_suspended)
>> - return;
>> console_locked = 1;
>> console_may_schedule = 1;
>> }
>> @@ -2645,10 +2667,6 @@ int console_trylock(void)
>> return 0;
>> if (down_trylock_console_sem())
>> return 0;
>> - if (console_suspended) {
>> - up_console_sem();
>> - return 0;
>> - }
>> console_locked = 1;
>> console_may_schedule = 0;
>> return 1;
>
> Interesting. console_locked previously would not be set if
> console is suspended, but now it's always set, which in theory
> changes the way WARN_CONSOLE_UNLOCKED() macro works in some
> cases?
Yes, Petr mentioned [0] this during the v1 review. His direct comment:
"console_locked" seems to be used only in WARN_CONSOLE_UNLOCKED().
I could imagine a corner case where, for example, "vt" code does
not print the warning because it works as it works. But it does
not make much sense. IMHO, such a code should get fixed. And it
is just a warning after all.
And his final comment in that thread:
I believe that @console_suspended is not longer needed.
Let's replace it with the per-console flag and do not worry
about races.
John Ogness
[0] https://lore.kernel.org/lkml/ZAieQtcs7YEuCCDa@alley
On (23/07/11 17:29), John Ogness wrote:
[..]
> > Interesting. console_locked previously would not be set if
> > console is suspended, but now it's always set, which in theory
> > changes the way WARN_CONSOLE_UNLOCKED() macro works in some
> > cases?
>
> Yes, Petr mentioned [0] this during the v1 review. His direct comment:
>
> "console_locked" seems to be used only in WARN_CONSOLE_UNLOCKED().
> I could imagine a corner case where, for example, "vt" code does
> not print the warning because it works as it works. But it does
> not make much sense. IMHO, such a code should get fixed. And it
> is just a warning after all.
>
> And his final comment in that thread:
>
> I believe that @console_suspended is not longer needed.
> Let's replace it with the per-console flag and do not worry
> about races.
Oh, thanks for the pointers!
> [0] https://lore.kernel.org/lkml/ZAieQtcs7YEuCCDa@alley
On (23/07/10 15:51), John Ogness wrote:
> Currently the global @console_suspended is used to determine if
> consoles are in a suspended state. Its primary purpose is to allow
> usage of the console_lock when suspended without causing console
> printing. It is synchronized by the console_lock.
>
> Rather than relying on the console_lock to determine suspended
> state, make it an official per-console state that is set within
> console->flags. This allows the state to be queried via SRCU.
>
> Remove @console_suspended. Console printing will still be avoided
> when suspended because console_is_usable() returns false when
> the new suspended flag is set for that console.
>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On (23/07/11 17:43), Petr Mladek wrote:
> On Mon 2023-07-10 15:51:21, John Ogness wrote:
> > The printk path is NMI safe because it only adds content to the
> > buffer and then triggers the delayed output via irq_work. If the
> > console is flushed or unblanked on panic (from NMI context) then it
> > can deadlock in down_trylock_console_sem() because the semaphore is
> > not NMI safe.
>
> <thinking loudly>
>
> Just to be sure. The semaphore is not NMI safe because even the
> trylock takes an internal spin lock. Am I right, please?
>
> Alternative solution would be to make down_trylock() NMI safe
> by using raw_spin_trylock_irqsave() for the internal lock.
>
> But this actually would not solve the whole problem. If the NMI safe
> down_trylock() succeeded then up() would need to be called
> in NMI as well. And up() really needs to take the spin lock
> which might get blocked in the meantime.
I guess another problem with up() is that it also may call
try_to_wake_up(), that may attempt to acquire a bunch of other
spin_lock-s.
On Mon 2023-07-10 15:51:21, John Ogness wrote:
> The printk path is NMI safe because it only adds content to the
> buffer and then triggers the delayed output via irq_work. If the
> console is flushed or unblanked on panic (from NMI context) then it
> can deadlock in down_trylock_console_sem() because the semaphore is
> not NMI safe.
<thinking loudly>
Just to be sure. The semaphore is not NMI safe because even the
trylock takes an internal spin lock. Am I right, please?
Alternative solution would be to make down_trylock() NMI safe
by using raw_spin_trylock_irqsave() for the internal lock.
But this actually would not solve the whole problem. If the NMI safe
down_trylock() succeeded then up() would need to be called
in NMI as well. And up() really needs to take the spin lock
which might get blocked in the meantime.
Another question is whether we want to call c->unblank()
in NMI even when down_trylock() was NMI safe. It seems that it
is implemented only for struct console vt_console_driver.
I am pretty sure that it takes more internal locks which
are not NMI safe either.
On the other hand, if we would risk calling c->write() then
we might risk calling c->unblank() either.
Finally, it is not only about NMI. Any locks might cause a deadlock
in panic() in any context. It is because other CPUs are stopped
and might block some locks.
</thinking loudly>
In my opinion, we should handle c->unblank() in panic() the same way
as c->write() in panic().
I suggest to create
void __console_unblank(void)
{
struct console *c;
int cookie;
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
if ((console_srcu_read_flags(c) & CON_ENABLED) && c->unblank)
c->unblank();
}
console_srcu_read_unlock(cookie);
}
and call this in console_flush_on_panic() without the console_lock().
We still need to take the lock during Oops when the system tries
to continue. In this case, the NMI check makes perfect sense.
NMI might cause a deadlock. Other contexts should be safe
because the CPUs are not stopped.
> Avoid taking the console lock when flushing in panic. To prevent
> other CPUs from taking the console lock while flushing, have
> console_lock() block and console_trylock() fail for non-panic CPUs
> during panic.
I really like the trick that console_lock() and console_trylock()
would start failing on non-panic CPUs. It should prevent races
when the other CPUs were not stopped for some reasons.
I am still slightly afraid to do this even before stopping other CPUs.
But I do not have any real scenario where it might be a problem.
And it is only about console_lock() which should preferably be
available for the panic-CPU. Also we should _not_ rely on the other
CPUs during panic() anyway. So, it should be fine after all.
Well, would you mind to split this into two patches?
1st patch would split __console_unblank(), call it from
console_flush_on_panic() after the trylock().
Also it would add the NMI check into the original
console_unblank() which would still be called in
bust_spinlocks().
The commit message should explain the motivation
(primary the internal spinlock in the semaphore
implementation). Also it should explain why only NMI
is a problem when called in the Oops path.
And why the locks are problem in any context
when called in panic() after CPUs were stopped.
2nd patch would prevent taking console_lock on non-panic CPUs.
And it would remove console_trylock()/console_unlock() from
console_flush_on_panic().
The commit message should explain the motivation
(the internal spinlock in the semaphore code).
Also it would solve a problem with a potential
double unlock. And it should mention that it should
not be worse then before when the trylock() result
was ignored.
IMHO, both patches has a potential to cause regressions.
And it is better to do it in smaller steps.
Best Regards,
Petr
On 2023-07-11, Petr Mladek <[email protected]> wrote:
> Just to be sure. The semaphore is not NMI safe because even the
> trylock takes an internal spin lock. Am I right, please?
Yes, that is one of the reasons. Sergey mentioned another (waking a task
on up()).
> Alternative solution would be to make down_trylock() NMI safe
> by using raw_spin_trylock_irqsave() for the internal lock.
NMI contexts are only allowed to take raw spinlocks if those spinlocks
are only used from NMI context. Otherwise you could have deadlock:
raw_spin_lock()
--- NMI ---
raw_spin_lock()
Using a trylock does not avoid the deadlock danger.
> Another question is whether we want to call c->unblank()
> in NMI even when down_trylock() was NMI safe. It seems that it
> is implemented only for struct console vt_console_driver.
> I am pretty sure that it takes more internal locks which
> are not NMI safe either.
Yes, it does. As an example, it calls mod_timer(), which is also not NMI
safe. Clearly the unblank() callback must not be called in NMI context.
> Finally, it is not only about NMI. Any locks might cause a deadlock
> in panic() in any context. It is because other CPUs are stopped
> and might block some locks.
With the atomic/threaded model this is not true. The port ownership can
be safely taken over from stopped CPUs.
> In my opinion, we should handle c->unblank() in panic() the same way
> as c->write() in panic().
I do not agree. Clearly unblank() is not NMI safe. Also, in current
mainline code, console_unblank() will already give up if the trylock
failed (rather than ignoring the lock, like write() does). So
console_unblank() might as well also give up if in NMI context.
John
On Mon 2023-07-10 15:51:22, John Ogness wrote:
> Printing to consoles can be deferred for several reasons:
>
> - explicitly with printk_deferred()
> - printk() in NMI context
> - recursive printk() calls
>
> The current implementation is not consistent. For printk_deferred(),
> irq work is scheduled twice. For NMI und recursive, panic CPU
> suppression and caller delays are not properly enforced.
>
> Correct these inconsistencies by consolidating the deferred printing
> code so that vprintk_deferred() is the top-level function for
> deferred printing and vprintk_emit() will perform whichever irq_work
> queueing is appropriate.
>
> Also add kerneldoc for wake_up_klogd() and defer_console_output() to
> clarify their differences and appropriate usage.
>
> Signed-off-by: John Ogness <[email protected]>
Looks good to me:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed 2023-07-12 23:17:49, John Ogness wrote:
> On 2023-07-11, Petr Mladek <[email protected]> wrote:
> > Just to be sure. The semaphore is not NMI safe because even the
> > trylock takes an internal spin lock. Am I right, please?
>
> Yes, that is one of the reasons. Sergey mentioned another (waking a task
> on up()).
I see.
> > Alternative solution would be to make down_trylock() NMI safe
> > by using raw_spin_trylock_irqsave() for the internal lock.
>
> NMI contexts are only allowed to take raw spinlocks if those spinlocks
> are only used from NMI context. Otherwise you could have deadlock:
>
> raw_spin_lock()
> --- NMI ---
> raw_spin_lock()
>
> Using a trylock does not avoid the deadlock danger.
>
> > Another question is whether we want to call c->unblank()
> > in NMI even when down_trylock() was NMI safe. It seems that it
> > is implemented only for struct console vt_console_driver.
> > I am pretty sure that it takes more internal locks which
> > are not NMI safe either.
>
> Yes, it does. As an example, it calls mod_timer(), which is also not NMI
> safe. Clearly the unblank() callback must not be called in NMI context.
>
> > Finally, it is not only about NMI. Any locks might cause a deadlock
> > in panic() in any context. It is because other CPUs are stopped
> > and might block some locks.
>
> With the atomic/threaded model this is not true. The port ownership can
> be safely taken over from stopped CPUs.
Right. But it would mean using the special lock also in c->unblank()
code. And it is only tty console which is one of the most complicated
consoles.
> > In my opinion, we should handle c->unblank() in panic() the same way
> > as c->write() in panic().
>
> I do not agree. Clearly unblank() is not NMI safe. Also, in current
> mainline code, console_unblank() will already give up if the trylock
> failed (rather than ignoring the lock, like write() does). So
> console_unblank() might as well also give up if in NMI context.
You are right.
OK, could we at least improve the commit message, please?
Something like:
<proposal>
console_sem() is not NMI safe even when using down_trylock(). Both
down_trylock() and up() are using an interal spinlock. up() might even
call wake_up_process() with another locks.
It is even worse in the panic() code path where the locks might be blocked
by stopped CPUs.
The sepaphore is used in two code paths, in console_unblank() and when
flushing console messages. On the low level, it is needed when calling
c->unblank() and c->write() callbacks.
Both code paths are not safe in panic() but they are handled differently.
c->unblank() is called only when console_trylock() succeeded. c->write()
is called in console_flush_on_panic() even when console_trylock().
The risk of a deadlock in c->write() callbacks is reduced by using trylock()
for the internal locks when oops_in_progess variable is set.
Reduce the risk of deadlocks caused the console semapthore by:
+ bailing out from console_unblank() in NMI
+ not taking the console_sem() in console_flush_on_panic() at all
Simple removal of console_trylock() in console_flush_on_panic() would
cause that other CPUs might still be able to take it and race.
The problem is avoided by checking panic_in_progress() in console_lock()
and console_trylock(). They will never succeed on non-panic CPUs.
The change is a preparation step for introducing printk kthreads and
atomic console write callbacks. It would make the panic() code path
completely safe for consoles without c->unblank() callback.
</proposal>
Wait, the last paragraph is not true. console_trylock() is still
called in console_unblank() in non-NMI context. But the lock
might be blocked by a stopped CPU.
It can be solved by checking whether there is any registered console
with c->unblank() callback first. As a result, console_trylock()
would be called only when a tty console is registered. The panic() path
really might be completely safe where only safe consoles are
registered.
It would make sense to do separate patch for console_unblank()
and console_flush_on_panic().
Of course, we might also improve console_unblank() later. But I
would still like to improve the commit message. You know, the original
commit title and message is talking about NMI. But the patch
has effects even in non-NMI context.
Best Regards,
Petr
On Mon 2023-07-10 15:51:23, John Ogness wrote:
> Currently the global @console_suspended is used to determine if
> consoles are in a suspended state. Its primary purpose is to allow
> usage of the console_lock when suspended without causing console
> printing. It is synchronized by the console_lock.
>
> Rather than relying on the console_lock to determine suspended
> state, make it an official per-console state that is set within
> console->flags. This allows the state to be queried via SRCU.
>
> Remove @console_suspended. Console printing will still be avoided
> when suspended because console_is_usable() returns false when
> the new suspended flag is set for that console.
>
> Signed-off-by: John Ogness <[email protected]>
Looks good to me:
Reviewed-by: Petr Mladek <[email protected]>
I have double checked the history. suspend_console() was added
into v2.6.18-rc1 by the commit ("Add support for suspending and
resuming the whole console subsystem").
The above commit added "secondary_console_sem". It was taken
by acquire_console_sem() instead of the normal "console_sem"
when "console_suspended" was set. It means that the normal
"console_sem" really was not taken.
The "secondary_console_sem" was removed in v2.6.29-rc6 by the commit
("PM: Fix suspend_console and resume_console to use only one
semaphore"). It solved races between code taking "console_sem"
and code "secondary_console_sem". This commit kept the handling of
"console_locked". It was not set when console_suspended was set
even though "console_sem" was actually taken.
IMHO, it was a bug in the commit removing "secondary_console_sem".
But it probably never caused any issues.
Anyway, this patch makes "console_locked" handling sane. And if some
tty code relies on the insane logic then it should get fixed.
Best Regards,
Petr
On Mon 2023-07-10 15:51:24, John Ogness wrote:
> Currently abandon_console_lock_in_panic() is only used to determine if
> the current CPU should immediately release the console lock because
> another CPU is in panic. However, later this function will be used by
> the CPU to immediately release other resources in this situation.
>
> Rename the function to other_cpu_in_panic(), which is a better
> description and does not assume it is related to the console lock.
>
> Signed-off-by: John Ogness <[email protected]>
Looks good:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On (23/07/13 16:43), Petr Mladek wrote:
>
> Simple removal of console_trylock() in console_flush_on_panic() would
> cause that other CPUs might still be able to take it and race.
> The problem is avoided by checking panic_in_progress() in console_lock()
> and console_trylock(). They will never succeed on non-panic CPUs.
>
In theory, we also can have non-panic CPU in console_flush_all(),
which should let panic CPU to take over the next time it checks
abandon_console_lock_in_panic() (other_cpu_in_panic() after 5/5),
but it may not happen immediately. I wonder if we somehow can/want
to "wait" in console_flush_on_panic() for non-panic CPU handover?
On Fri 2023-07-14 13:00:49, Sergey Senozhatsky wrote:
> On (23/07/13 16:43), Petr Mladek wrote:
> >
> > Simple removal of console_trylock() in console_flush_on_panic() would
> > cause that other CPUs might still be able to take it and race.
> > The problem is avoided by checking panic_in_progress() in console_lock()
> > and console_trylock(). They will never succeed on non-panic CPUs.
> >
>
> In theory, we also can have non-panic CPU in console_flush_all(),
> which should let panic CPU to take over the next time it checks
> abandon_console_lock_in_panic() (other_cpu_in_panic() after 5/5),
> but it may not happen immediately. I wonder if we somehow can/want
> to "wait" in console_flush_on_panic() for non-panic CPU handover?
Good point. It might actually be any console_lock() owner,
including printk() on other CPU.
I think that we might need to add some wait() as we did in the last
attempt, see the commit b87f02307d3cfbda76852 ("printk: Wait for
the global console lock when the system is going down").
Anyway, it will be more important after introducing the kthreads.
There is a non-trivial chance that they would block the lock.
They might be busy when handling a message printed right before
the panic() was called. At least, this is what I saw in the last
attempt to introduce the kthreads.
But maybe, it will be somehow hidden in the new atomic lock.
It might be passed to a printk context with a higher priority
and it uses some wait internally, see the waiting in the patch
https://lore.kernel.org/all/[email protected]/
Anyway, this patch does not make it worse. It just ignores the
potential console_lock owner in console_flush_on_panic() another
way.
Best Regards,
Petr