From: Daniel Thompson <hidden> Date: 2015-12-18 10:18:18
On 11/12/15 23:26, Jiri Kosina wrote:
On Fri, 11 Dec 2015, Russell King - ARM Linux wrote:
quoted
I'm personally happy with the existing code, and I've been wondering why
there's this effort to apply further cleanups - to me, the changelogs
don't seem to make that much sense, unless we want to start using
printk() extensively in NMI functions - using the generic nmi backtrace
code surely gets us something that works across all architectures...
It is already being used extensively, and not only for all-CPU backtraces.
For starters, please consider
- WARN_ON(in_nmi())
- BUG_ON(in_nmi())
Sorry to join in so late but...
Today we risk deadlock when we try to issue these diagnostic errors
directly from NMI context.
After this change we will still risk deadlock, because that's what the
diagnostic code is trying to tell us, *and* we delay actually reporting
the error until, and only if, the NMI handler completes.
I'm not entirely sure that this is an improvement.
- anything being printed out from MCE handlers
The MCE handlers should only call printk() when they decide to panic and
*after* busting the spinlocks. At this point deferring printk() until it
is safe is not very helpful.
When we bust the spinlocks we should probably restore the normal
printk() function to give best chance of the failure messages making it out.
Daniel.
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-12-18 11:29:29
On Fri, Dec 18, 2015 at 10:18:08AM +0000, Daniel Thompson wrote:
I'm not entirely sure that this is an improvement.
What I do these days is delete everything in vprintk_emit() and simply
call early_printk().
Kill the useless kmsg buffer crap and locking, just pound bytes to the
UART registers without anything in between.
The other semi usable solution is redirecting to trace_printk() and
recovering the trace buffers from your kdump. But I've found that
typically kdump doesn't work anymore if you properly wedge the machine.
So this is very much a second rate solution.
But this globally locked buffer, calling out to console drivers that do
locking and even scheduling, is an unreliable unfixable trainwreck that
I've given up on.
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-12-18 12:11:53
On Fri, Dec 18, 2015 at 12:29:02PM +0100, Peter Zijlstra wrote:
On Fri, Dec 18, 2015 at 10:18:08AM +0000, Daniel Thompson wrote:
quoted
I'm not entirely sure that this is an improvement.
What I do these days is delete everything in vprintk_emit() and simply
call early_printk().
On that, whoever made the device model use vprintk_emit() broke the
debugger (KGDB/KDB) printk intercept, and the whole vprintk_func
redirection scheme.
From: Petr Mladek <pmladek@suse.com> Date: 2015-12-18 14:52:15
On Fri 2015-12-18 10:18:08, Daniel Thompson wrote:
On 11/12/15 23:26, Jiri Kosina wrote:
quoted
On Fri, 11 Dec 2015, Russell King - ARM Linux wrote:
quoted
I'm personally happy with the existing code, and I've been wondering why
there's this effort to apply further cleanups - to me, the changelogs
don't seem to make that much sense, unless we want to start using
printk() extensively in NMI functions - using the generic nmi backtrace
code surely gets us something that works across all architectures...
It is already being used extensively, and not only for all-CPU backtraces.
For starters, please consider
- WARN_ON(in_nmi())
- BUG_ON(in_nmi())
Sorry to join in so late but...
Today we risk deadlock when we try to issue these diagnostic errors
directly from NMI context.
After this change we will still risk deadlock, because that's what
the diagnostic code is trying to tell us, *and* we delay actually
reporting the error until, and only if, the NMI handler completes.
I think that NMI messages about a possible deadlock are the ones
from
kernel/locking/rtmutex.c
kernel/irq_work.c
include/linux/hardirq.h
You are right that if the deadlock happens, this patch set lowers the
chance to see the message.
On the other hand, all the other printk's in NMI seems to be non-fatal
warnings. In this case, this patch set increases the chance to see
them.
A compromise might be to explicitly call printk_nmi_flush() in the few
fatal cases. Alternatively we could force the messages on the
early_console when available.
quoted
- anything being printed out from MCE handlers
The MCE handlers should only call printk() when they decide to panic
and *after* busting the spinlocks. At this point deferring printk()
until it is safe is not very helpful.
When we bust the spinlocks we should probably restore the normal
printk() function to give best chance of the failure messages making
it out.
The problem is that we do not know what locks need to be busted. There
are too many consoles and too many locks involved. Also busting locks
open another can of worms.
Best Regards,
Petr
From: Daniel Thompson <hidden> Date: 2015-12-18 17:00:23
On 18/12/15 14:52, Petr Mladek wrote:
On Fri 2015-12-18 10:18:08, Daniel Thompson wrote:
quoted
On 11/12/15 23:26, Jiri Kosina wrote:
quoted
On Fri, 11 Dec 2015, Russell King - ARM Linux wrote:
quoted
I'm personally happy with the existing code, and I've been wondering why
there's this effort to apply further cleanups - to me, the changelogs
don't seem to make that much sense, unless we want to start using
printk() extensively in NMI functions - using the generic nmi backtrace
code surely gets us something that works across all architectures...
It is already being used extensively, and not only for all-CPU backtraces.
For starters, please consider
- WARN_ON(in_nmi())
- BUG_ON(in_nmi())
Sorry to join in so late but...
Today we risk deadlock when we try to issue these diagnostic errors
directly from NMI context.
After this change we will still risk deadlock, because that's what
the diagnostic code is trying to tell us, *and* we delay actually
reporting the error until, and only if, the NMI handler completes.
I think that NMI messages about a possible deadlock are the ones
from
kernel/locking/rtmutex.c
kernel/irq_work.c
include/linux/hardirq.h
You are right that if the deadlock happens, this patch set lowers the
chance to see the message.
On the other hand, all the other printk's in NMI seems to be non-fatal
warnings. In this case, this patch set increases the chance to see
them.
Maybe for a WARN_ON() the trade off is worth it but I don't think a
BUG_ON() trace would ever make it out.
A compromise might be to explicitly call printk_nmi_flush() in the few
fatal cases. Alternatively we could force the messages on the
early_console when available.
quoted
quoted
- anything being printed out from MCE handlers
The MCE handlers should only call printk() when they decide to panic
and *after* busting the spinlocks. At this point deferring printk()
until it is safe is not very helpful.
When we bust the spinlocks we should probably restore the normal
printk() function to give best chance of the failure messages making
it out.
The problem is that we do not know what locks need to be busted. There
are too many consoles and too many locks involved. Also busting locks
open another can of worms.
Yes, I agree that busting the spinlocks doesn't avoid all risk of deadlock.
Probably I've been placing too much weight on the importance of getting
messages out when dying. You're right that surviving far enough through
a panic to trigger kdump or reset is equally (or more) important in many
scenarios than getting a failure message out.
However on a system with nothing but "while(1) {}" hooked up to panic()
then its worth risking a lock up. In this case restoring normal printk()
behavior and dumping the NMI buffers would be worthwhile.
Daniel.
From: Andrew Morton <akpm@linux-foundation.org> Date: 2015-12-18 23:03:24
On Fri, 18 Dec 2015 13:11:41 +0100 Peter Zijlstra [off-list ref] wrote:
On Fri, Dec 18, 2015 at 12:29:02PM +0100, Peter Zijlstra wrote:
quoted
On Fri, Dec 18, 2015 at 10:18:08AM +0000, Daniel Thompson wrote:
quoted
I'm not entirely sure that this is an improvement.
What I do these days is delete everything in vprintk_emit() and simply
call early_printk().
On that, whoever made the device model use vprintk_emit() broke the
debugger (KGDB/KDB) printk intercept, and the whole vprintk_func
redirection scheme.
crap, we have a whole set of interfaces which are broken this way.
printk_emit(), vprintk(), vprintk_emit().
commit 7ff9554bb578ba02166071d2d487b7fc7d860d62
Author: Kay Sievers [off-list ref]
AuthorDate: Thu May 3 02:29:13 2012 +0200
Commit: Greg Kroah-Hartman [off-list ref]
CommitDate: Mon May 7 16:53:02 2012 -0700
printk: convert byte-buffer to variable-length record buffer
From: Daniel Thompson <hidden> Date: 2016-03-01 14:05:12
On 18/12/15 17:00, Daniel Thompson wrote:
quoted
quoted
The MCE handlers should only call printk() when they decide to panic
and *after* busting the spinlocks. At this point deferring printk()
until it is safe is not very helpful.
When we bust the spinlocks we should probably restore the normal
printk() function to give best chance of the failure messages making
it out.
The problem is that we do not know what locks need to be busted. There
are too many consoles and too many locks involved. Also busting locks
open another can of worms.
Yes, I agree that busting the spinlocks doesn't avoid all risk of deadlock.
Probably I've been placing too much weight on the importance of getting
messages out when dying. You're right that surviving far enough through
a panic to trigger kdump or reset is equally (or more) important in many
scenarios than getting a failure message out.
However on a system with nothing but "while(1) {}" hooked up to panic()
then its worth risking a lock up. In this case restoring normal printk()
behavior and dumping the NMI buffers would be worthwhile.
An a (much) later thread[1] Andrew Morton described this comment as
non-committal. Sorry for that.
I don't object to the overall approach taken by Petr, merely that I
think there are cases where the current patchset doesn't put in quite
enough effort to issue messages.
Panic triggered during NMI is the only case I can think of and that, I
think, could be addressed by adding an extra call to printk_nmi_flush()
during panic(). It should probably only cover cases where we *don't*
call kdump and the panic handler doesn't restart the machine... so just
after the pr_emerg("...end kernel panic") would be OK for me.
Daniel.
[1]: http://thread.gmane.org/gmane.linux.ports.arm.kernel/482845