When a caller writes heavily to the kernel log (e.g. writing to
/dev/kmsg in a loop) while another panics, there's currently a high
likelihood of a deadlock (see patch 2 for the full description of this
deadlock).
The principle fix is to disable the optimistic spin once panic_cpu is
set, so the panic CPU doesn't spin waiting for a halted CPU to hand over
the console_sem.
However, this exposed us to a livelock situation, where the panic CPU
holds the console_sem, and another CPU could fill up the log buffer
faster than the consoles could drain it, preventing the panic from
progressing and halting the other CPUs. To avoid this, patch 3 adds a
mechanism to suppress printk (from non-panic-CPU) during panic, if we
reach a threshold of dropped messages.
A major goal with all of these patches is to try to decrease the
likelihood that another CPU is holding the console_sem when we halt it
in panic(). This reduces the odds of needing to break locks and
potentially encountering further deadlocks with the console drivers.
To test, I use the following script, kmsg_panic.sh:
#!/bin/bash
date
# 991 chars (based on log buffer size):
chars="$(printf 'a%.0s' {1..991})"
while :; do
echo $chars > /dev/kmsg
done &
echo c > /proc/sysrq-trigger &
date
exit
I defined a hang as any time the system did not reboot to a login prompt
on the serial console within 60 seconds. Here are the statistics on
hangs using this script, before and after the patch.
before: 776 hangs / 1484 trials - 52.3%
after : 0 hangs / 408 trials - 0.0%
v2:
Each patch has minor updates from code reviews. I've re-done testing and
updated the above statistics. Exact changes are in each patch.
Stephen Brennan (4):
panic: Add panic_in_progress helper
printk: disable optimistic spin during panic
printk: Avoid livelock with heavy printk during panic
printk: Drop console_sem during panic
include/linux/panic.h | 6 +++++
kernel/printk/printk.c | 51 +++++++++++++++++++++++++++++++++++++++++-
2 files changed, 56 insertions(+), 1 deletion(-)
--
2.30.2
If another CPU is in panic, we are about to be halted. Try to gracefully
abandon the console_sem, leaving it free for the panic CPU to grab.
Suggested-by: Petr Mladek <[email protected]>
Signed-off-by: Stephen Brennan <[email protected]>
---
Notes:
v2: Factor check out to a helper, and check at the end of
console_unlock() to prevent retry as well.
kernel/printk/printk.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 18107db118d4..572363ff716f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2593,6 +2593,25 @@ static int have_callable_console(void)
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();
+}
+
/*
* Can we actually use the console at this time on this cpu?
*
@@ -2742,6 +2761,10 @@ void console_unlock(void)
if (handover)
return;
+ /* Allow panic_cpu to take over the consoles safely */
+ if (abandon_console_lock_in_panic())
+ break;
+
if (do_cond_resched)
cond_resched();
}
@@ -2759,7 +2782,7 @@ void console_unlock(void)
* flush, no worries.
*/
retry = prb_read_valid(prb, next_seq, NULL);
- if (retry && console_trylock())
+ if (retry && !abandon_console_lock_in_panic() && console_trylock())
goto again;
}
EXPORT_SYMBOL(console_unlock);
--
2.30.2
On 2022-01-26, Stephen Brennan <[email protected]> wrote:
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2759,7 +2782,7 @@ void console_unlock(void)
> * flush, no worries.
> */
> retry = prb_read_valid(prb, next_seq, NULL);
> - if (retry && console_trylock())
> + if (retry && !abandon_console_lock_in_panic() && console_trylock())
As Sergey suggested [0], I would like to see the call to
abandon_console_lock_in_panic() move inside console_trylock(). This will
help to avoid the race between NMI CPU halt and the internal sema.lock
spinlock.
John Ogness
[0] https://lore.kernel.org/all/YfJFjHdg%[email protected]
On Thu 2022-01-27 10:28:53, John Ogness wrote:
> On 2022-01-26, Stephen Brennan <[email protected]> wrote:
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2759,7 +2782,7 @@ void console_unlock(void)
> > * flush, no worries.
> > */
> > retry = prb_read_valid(prb, next_seq, NULL);
> > - if (retry && console_trylock())
> > + if (retry && !abandon_console_lock_in_panic() && console_trylock())
>
> As Sergey suggested [0], I would like to see the call to
> abandon_console_lock_in_panic() move inside console_trylock(). This will
> help to avoid the race between NMI CPU halt and the internal sema.lock
> spinlock.
I would prefer if it is done as a followup patch. The code in this
patch is still needed. It helps when the non-panic CPU is busy
with pushing many pending messages. Also it is a more conservative
approach.
Always failing console_trylock() on non-panic CPU makes sense as well.
But it affects many more users. It is likely safe because it is
trylock. But the entire effect is not fully clear to me. So, I suggest
to do it in a separate patch. It might help with bisection.
Best Regards,
Petr
On Wed 2022-01-26 15:02:36, Stephen Brennan wrote:
> If another CPU is in panic, we are about to be halted. Try to gracefully
> abandon the console_sem, leaving it free for the panic CPU to grab.
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 18107db118d4..572363ff716f 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2742,6 +2761,10 @@ void console_unlock(void)
> if (handover)
> return;
>
> + /* Allow panic_cpu to take over the consoles safely */
> + if (abandon_console_lock_in_panic())
> + break;
Hmm, it makes some sense to have it before cond_resched(). But I would
like to have it at the beginning of the cycle so that console_unlock()
might leave quickly without processing any single message.
We could have it in both (three) locations. But it might be over
cautious.
Anyway, the beginning is more important. Sleeping with console_sem is
less risky from the panic and races point of view.
Best Regards,
Petr
On (22/01/27 16:03), Petr Mladek wrote:
> On Thu 2022-01-27 10:28:53, John Ogness wrote:
> > On 2022-01-26, Stephen Brennan <[email protected]> wrote:
> > > --- a/kernel/printk/printk.c
> > > +++ b/kernel/printk/printk.c
> > > @@ -2759,7 +2782,7 @@ void console_unlock(void)
> > > * flush, no worries.
> > > */
> > > retry = prb_read_valid(prb, next_seq, NULL);
> > > - if (retry && console_trylock())
> > > + if (retry && !abandon_console_lock_in_panic() && console_trylock())
> >
> > As Sergey suggested [0], I would like to see the call to
> > abandon_console_lock_in_panic() move inside console_trylock(). This will
> > help to avoid the race between NMI CPU halt and the internal sema.lock
> > spinlock.
Thanks John.
> I would prefer if it is done as a followup patch. The code in this
> patch is still needed. It helps when the non-panic CPU is busy
> with pushing many pending messages. Also it is a more conservative
> approach.
No objections. This series fixes issue at hand, so conservative approach
makes sense.
On the other hand, we are at -rc1 and it seems like a very good time to
discuss/look into/work on/etc. solution for the remaining cases/races/etc.