[RFC][PATCH -next] make pstore/kmsg_dump run after stopping other cpus in panic path

Hi,

As Don mentioned in following thread, it would be nice for pstore/kmsg_dump to serialize
panic path and have one cpu running because they can log messages reliably.

https://lkml.org/lkml/2011/10/13/427

For realizing this idea, we have to move kmsg_dump below smp_send_stop() and bust some locks
of kmsg_dump/pstore in panic path.

This patch does followings.

- moving kmsg_dump(KMSG_DUMP_PANIC) below smp_send_stop.
- busting logbuf_lock of kmsg_dump() in panic path for avoiding deadlock.
- busting psinfo->buf_lock of pstore_dump() in panic path for avoiding deadlock.

Any comments are welcome.

Hi,

As Don mentioned in following thread, it would be nice for pstore/kmsg_dump to serialize
panic path and have one cpu running because they can log messages reliably.

https://lkml.org/lkml/2011/10/13/427

For realizing this idea, we have to move kmsg_dump below smp_send_stop() and bust some locks
of kmsg_dump/pstore in panic path.

This patch does followings.

- moving kmsg_dump(KMSG_DUMP_PANIC) below smp_send_stop.
- busting logbuf_lock of kmsg_dump() in panic path for avoiding deadlock.
- busting psinfo->buf_lock of pstore_dump() in panic path for avoiding deadlock.

Any comments are welcome.

Signed-off-by: Seiji Aguchi


fs/pstore/platform.c | 22 ++++++++++
kernel/panic.c | 4 ++--
kernel/printk.c | 7 +++++++
3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 2bd620f..e73d940 100644
a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -90,19 +90,21 @@ static void pstore_dump(struct kmsg_dumper *dumper,
int hsize, ret;
unsigned int part = 1;
unsigned long flags = 0;
- int is_locked = 0;

if (reason < ARRAY_SIZE(reason_str))
why = reason_str[reason];
else
why = "Unknown";

- if (in_nmi()) {
- is_locked = spin_trylock(&psinfo->buf_lock);
- if (!is_locked)
- pr_err("pstore dump routine blocked in NMI, may corrupt error record");
- } else
- spin_lock_irqsave(&psinfo->buf_lock, flags);
+ /*
+ * pstore_dump() is called after smp_send_stop() in panic path.
+ * So, spin_lock should be bust for avoiding deadlock.
+ */
+ if (reason == KMSG_DUMP_PANIC)
+ spin_lock_init(&psinfo->buf_lock);
+
+ spin_lock_irqsave(&psinfo->buf_lock, flags);
+
oopscount++;
while (total < kmsg_bytes) {
dst = psinfo->buf;
@@ -131,11 +133,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
total += l1_cpy + l2_cpy;
part++;
}
- if (in_nmi()) {
- if (is_locked)
- spin_unlock(&psinfo->buf_lock);
- } else
- spin_unlock_irqrestore(&psinfo->buf_lock, flags);
+ spin_unlock_irqrestore(&psinfo->buf_lock, flags);
}

static struct kmsg_dumper pstore_dumper = {
diff --git a/kernel/panic.c b/kernel/panic.c
index d7bb697..41bf6ad 100644
a/kernel/panic.c
+++ b/kernel/panic.c
@@ -88,8 +88,6 @@ NORET_TYPE void panic(const char * fmt, ...)
*/
crash_kexec(NULL);

- kmsg_dump(KMSG_DUMP_PANIC);
-
/*
* Note smp_send_stop is the usual smp shutdown function, which
* unfortunately means it may not be hardened to work in a panic
@@ -97,6 +95,8 @@ NORET_TYPE void panic(const char * fmt, ...)
*/
smp_send_stop();

+ kmsg_dump(KMSG_DUMP_PANIC);
+
atomic_notifier_call_chain(&panic_notifier_list, 0, buf);

bust_spinlocks(0);

diff --git a/kernel/printk.c b/kernel/printk.c
index 1455a0d..e1e57db 100644
a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1732,6 +1732,13 @@ void kmsg_dump(enum kmsg_dump_reason reason)
unsigned long l1, l2;
unsigned long flags;

+ /*
+ * kmsg_dump() is called after smp_send_stop() in panic path.
+ * So, spin_lock should be bust for avoiding deadlock.
+ */
+ if (reason == KMSG_DUMP_PANIC)
+ raw_spin_lock_init(&logbuf_lock);
+
/* Theoretically, the log could move on after we do this, but
there's not a lot we can do about that. The new messages
will overwrite the start of what we dump. */

All this lock busting probably isn't pretty and causes one to reflect what
is going on here. But as long as we are going to keep the kmsg_dump
design, changes like this seem necessary to make sure the locking stays
sane(r) for now.

> All this lock busting probably isn't pretty and causes one to reflect what
> is going on here. But as long as we are going to keep the kmsg_dump
> design, changes like this seem necessary to make sure the locking stays
> sane(r) for now.

So should we let the back-end know that locks have been busted? I worry
that we'll get through the pstore layer by blasting away at any locks that
get in our way - but then the backend (which may well have been written on
the assumption that pstore serialized calls to it) will not do anything
useful (and may cause its own hang).

From: Don Zickus [mailto:]
Sent: Monday, October 17, 2011 1:22 PM
To: Luck, Tony
Cc: Seiji Aguchi; ; Matthew Garrett; Vivek Goyal; Chen, Gong; Andrew Morton; Brown, Len; Huang,
Ying; ; ; ; ; ;
; ; Satoru Moriya
Subject: Re: [RFC][PATCH -next] make pstore/kmsg_dump run after stopping other cpus in panic path

On Mon, Oct 17, 2011 at 09:56:50AM -0700, Luck, Tony wrote:

> All this lock busting probably isn't pretty and causes one to reflect what
> is going on here. But as long as we are going to keep the kmsg_dump
> design, changes like this seem necessary to make sure the locking stays
> sane(r) for now.

So should we let the back-end know that locks have been busted? I worry
that we'll get through the pstore layer by blasting away at any locks that
get in our way - but then the backend (which may well have been written on
the assumption that pstore serialized calls to it) will not do anything
useful (and may cause its own hang).

I was kinda alluding to something like that, when I said we probably need
follow on patches to clean up the backend. But maybe it would be smarter
to set a flag in pstore to let the backend know we busted the locks
instead of letting them do panic checks themselves. I agree with your
concerns.

I really can't think of a good overall solution other than slowly
step-by-step untangle things by making subtle changes, this round being
the serialization of kmsg_dump().

My brain is to small to figure this out. I keep thinking about making an
ascii art diagram to see all the various paths and their contexts, but
then I keep finding more interesting things to do. :-)

Perhaps we should do that and make some rules about what the back-end can
and can not do when plugging into kmsg_dump.

Cheers,
Don