[RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Seiji Aguchi]

Hi,

This patch adds static function calls so that both pstore and APEI storage backend can work reliably in kexec path.


 kernel/kexec.c
   - Add pstore_kmsg_dump_in_interrupt(KMSG_DUMP_KEXEC) just after machine_crash_shutdown() so that pstore can
     work with one cpu.

 kernel/printk.c
   - Introduce get_logbuf_nolock() so that pstore can get values of logbuf without taking lock.

 fs/pstore/platform.c
   - Introduce pstore_kmsg_dump_in_interrupt() so that pstore/APEI storage backend can output kernel messages without taking lock.
   pstore_dump()
   - Add error checks below because pstore_dump() is called from kmsg_dump(KMSG_DUMP_KEXEC) directly.
       - Skip if no driver is registered
       - Skip if there is a driver calling pstore_register()/pstore_unregister()
   - Remove mutex_lock from kexec path

TODO:
  APEI storage backend will work with this patch.
  However, I don't have any access to servers capable of APEI storage backend.
  Please help to test my patch.

 Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>

---
 fs/pstore/platform.c      |   27 +++++++++++++++++++++++++--
 include/linux/kmsg_dump.h |    1 +
 include/linux/pstore.h    |    9 +++++++++
 kernel/kexec.c            |    3 +++
 kernel/printk.c           |   29 +++++++++++++++++++++++++++++
 5 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index f2c3ff2..85e0a9c 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -74,7 +74,17 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 	else
 		why = "Unknown";
 
-	mutex_lock(&psinfo->buf_mutex);
+	switch (reason) {
+	case KMSG_DUMP_KEXEC:
+		/* Skip if there is no driver or there is a driver calling
+		   pstore_register() */
+		if (!psinfo || !spin_trylock(&pstore_lock))
+			return;
+		break;
+	default:
+		mutex_lock(&psinfo->buf_mutex);
+	}
+
 	oopscount++;
 	while (total < kmsg_bytes) {
 		dst = psinfo->buf;
@@ -103,7 +113,20 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		l2 -= l2_cpy;
 		total += l1_cpy + l2_cpy;
 	}
-	mutex_unlock(&psinfo->buf_mutex);
+
+	if (reason != KMSG_DUMP_KEXEC)
+		mutex_unlock(&psinfo->buf_mutex);
+}
+
+void pstore_kmsg_dump_in_interrupt(enum kmsg_dump_reason reason)
+{
+	const char *s1, *s2;
+	unsigned long l1, l2;
+
+	/* get logbuf values without spin_lock for avoiding dead lock */
+	get_logbuf_nolock(&s1, &l1, &s2, &l2);
+
+	pstore_dump(NULL, reason, s1, l1, s2, l2);
 }
 
 static struct kmsg_dumper pstore_dumper = {
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index fee6631..ee0c952 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -18,6 +18,7 @@
 enum kmsg_dump_reason {
 	KMSG_DUMP_OOPS,
 	KMSG_DUMP_PANIC,
+	KMSG_DUMP_KEXEC,
 	KMSG_DUMP_RESTART,
 	KMSG_DUMP_HALT,
 	KMSG_DUMP_POWEROFF,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 2455ef2..5cf008d 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -22,6 +22,8 @@
 #ifndef _LINUX_PSTORE_H
 #define _LINUX_PSTORE_H
 
+#include<linux/kmsg_dump.h>
+
 /* types */
 enum pstore_type_id {
 	PSTORE_TYPE_DMESG	= 0,
@@ -46,6 +48,9 @@ struct pstore_info {
 #ifdef CONFIG_PSTORE
 extern int pstore_register(struct pstore_info *);
 extern int pstore_write(enum pstore_type_id type, char *buf, size_t size);
+extern void pstore_kmsg_dump_in_interrupt(enum kmsg_dump_reason reason);
+extern void get_logbuf_nolock(const char **s1, unsigned long *l1,
+			      const char **s2, unsigned long *l2);
 #else
 static inline int
 pstore_register(struct pstore_info *psi)
@@ -57,6 +62,10 @@ pstore_write(enum pstore_type_id type, char *buf, size_t size)
 {
 	return -ENODEV;
 }
+static inline void
+pstore_kmsg_dump_in_interrupt(enum kmsg_dump_reason reason)
+{
+}
 #endif
 
 #endif /*_LINUX_PSTORE_H*/
diff --git a/kernel/kexec.c b/kernel/kexec.c
index e24bc1b..8e2761a 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -33,6 +33,8 @@
 #include <linux/vmalloc.h>
 #include <linux/swap.h>
 #include <linux/syscore_ops.h>
+#include <linux/kmsg_dump.h>
+#include <linux/pstore.h>
 
 #include <asm/page.h>
 #include <asm/uaccess.h>
@@ -1081,6 +1083,7 @@ void crash_kexec(struct pt_regs *regs)
 			crash_setup_regs(&fixed_regs, regs);
 			crash_save_vmcoreinfo();
 			machine_crash_shutdown(&fixed_regs);
+			pstore_kmsg_dump_in_interrupt(KMSG_DUMP_KEXEC);
 			machine_kexec(kexec_crash_image);
 		}
 		mutex_unlock(&kexec_mutex);
diff --git a/kernel/printk.c b/kernel/printk.c
index 37dff34..966a7d9 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1710,6 +1710,35 @@ int kmsg_dump_unregister(struct kmsg_dumper *dumper)
 }
 EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
 
+
+void get_logbuf_nolock(const char **s1, unsigned long *l1, const char **s2,
+		       unsigned long *l2)
+{
+	unsigned long end;
+	unsigned chars;
+
+	/* 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. */
+	end = log_end & LOG_BUF_MASK;
+	chars = logged_chars;
+
+	if (chars > end) {
+		*s1 = log_buf + log_buf_len - chars + end;
+		*l1 = chars - end;
+
+		*s2 = log_buf;
+		*l2 = end;
+	} else {
+		*s1 = "";
+		*l1 = 0;
+
+		*s2 = log_buf + end - chars;
+		*l2 = chars;
+	}
+
+}
+
 /**
  * kmsg_dump - dump kernel log to kernel message dumpers.
  * @reason: the reason (oops, panic etc) for dumping
-- 
1.7.1

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Matthew Garrett]

On Tue, Jul 19, 2011 at 02:24:27PM -0400, Seiji Aguchi wrote:

> -	mutex_lock(&psinfo->buf_mutex);
> +	switch (reason) {
> +	case KMSG_DUMP_KEXEC:
> +		/* Skip if there is no driver or there is a driver calling
> +		   pstore_register() */
> +		if (!psinfo || !spin_trylock(&pstore_lock))
> +			return;
> +		break;
> +	default:
> +		mutex_lock(&psinfo->buf_mutex);

How is this safe? What happens if there's a pstore access in process 
when you hit this codepath?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Seiji Aguchi]

>How is this safe? What happens if there's a pstore access in process
>when you hit this codepath?

This will never happen because pstore_kmsg_dump_in_interrupt() is called after machine_crash_shutdown().

Panicked cpu sends NMI to all other cpus in machine_crash_shutdown() and they stop.


@@ -1081,6 +1083,7 @@ void crash_kexec(struct pt_regs *regs)
 			crash_setup_regs(&fixed_regs, regs);
 			crash_save_vmcoreinfo();
 			machine_crash_shutdown(&fixed_regs);
+			pstore_kmsg_dump_in_interrupt(KMSG_DUMP_KEXEC);

Seiji

>-----Original Message-----
>From: Matthew Garrett [mailto:mjg@redhat.com]
>Sent: Tuesday, July 19, 2011 2:35 PM
>To: Seiji Aguchi
>Cc: kexec@lists.infradead.org; linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org; Eric W. Biederman; Vivek
>Goyal; KOSAKI Motohiro; Americo Wang; tony.luck@intel.com; Andrew Morton; Jarod Wilson; hpa@zytor.com; dzickus@redhat.com;
>dle-develop@lists.sourceforge.net; Satoru Moriya
>Subject: Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path
>
>On Tue, Jul 19, 2011 at 02:24:27PM -0400, Seiji Aguchi wrote:
>
>> -	mutex_lock(&psinfo->buf_mutex);
>> +	switch (reason) {
>> +	case KMSG_DUMP_KEXEC:
>> +		/* Skip if there is no driver or there is a driver calling
>> +		   pstore_register() */
>> +		if (!psinfo || !spin_trylock(&pstore_lock))
>> +			return;
>> +		break;
>> +	default:
>> +		mutex_lock(&psinfo->buf_mutex);
>
>How is this safe? What happens if there's a pstore access in process
>when you hit this codepath?
>
>--
>Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Matthew Garrett]

On Tue, Jul 19, 2011 at 02:48:22PM -0400, Seiji Aguchi wrote:
> >How is this safe? What happens if there's a pstore access in process
> >when you hit this codepath?
> 
> This will never happen because pstore_kmsg_dump_in_interrupt() is called after machine_crash_shutdown().
> 
> Panicked cpu sends NMI to all other cpus in machine_crash_shutdown() and they stop.

And how does that handle the case where we're halfway through a pstore
access when the NMI arrives? ERST, at least, has a complex state 
machine. You can't guarantee what starting one transaction without 
completing one that was in process will do.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Don Zickus]

On Tue, Jul 19, 2011 at 02:48:22PM -0400, Seiji Aguchi wrote:
> >How is this safe? What happens if there's a pstore access in process
> >when you hit this codepath?
> 
> This will never happen because pstore_kmsg_dump_in_interrupt() is called after machine_crash_shutdown().
> 
> Panicked cpu sends NMI to all other cpus in machine_crash_shutdown() and they stop.
> 
> 
> @@ -1081,6 +1083,7 @@ void crash_kexec(struct pt_regs *regs)
>  			crash_setup_regs(&fixed_regs, regs);
>  			crash_save_vmcoreinfo();
>  			machine_crash_shutdown(&fixed_regs);
> +			pstore_kmsg_dump_in_interrupt(KMSG_DUMP_KEXEC);
> 
> Seiji

Another interesting question is do we need to log anything in the kdump
path?  Isn't kdump generating the same info?  What added value do we get
over kdump?

Cheers,
Don

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Vivek Goyal]

On Tue, Jul 19, 2011 at 02:54:11PM -0400, Don Zickus wrote:
> On Tue, Jul 19, 2011 at 02:48:22PM -0400, Seiji Aguchi wrote:
> > >How is this safe? What happens if there's a pstore access in process
> > >when you hit this codepath?
> > 
> > This will never happen because pstore_kmsg_dump_in_interrupt() is called after machine_crash_shutdown().
> > 
> > Panicked cpu sends NMI to all other cpus in machine_crash_shutdown() and they stop.
> > 
> > 
> > @@ -1081,6 +1083,7 @@ void crash_kexec(struct pt_regs *regs)
> >  			crash_setup_regs(&fixed_regs, regs);
> >  			crash_save_vmcoreinfo();
> >  			machine_crash_shutdown(&fixed_regs);
> > +			pstore_kmsg_dump_in_interrupt(KMSG_DUMP_KEXEC);
> > 
> > Seiji
> 
> Another interesting question is do we need to log anything in the kdump
> path?  Isn't kdump generating the same info?  What added value do we get
> over kdump?

I had the same question. The argument is that kdump can fail and they
can not afford to not capture any info at all. So before kdump executes
they want to save some state to NVRAM.

I am wondering that saving this info to NVRAM, can it be done early in
second kernel? I guess we are not supposed to take any EFI services in
second kernel so it might not be possible.

Thanks
Vivek

RE: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Seiji Aguchi]

>And how does that handle the case where we're halfway through a pstore
>access when the NMI arrives? ERST, at least, has a complex state
>machine. You can't guarantee what starting one transaction without
>completing one that was in process will do.

As for ERST, write access is protected by raw_spin_trylock_irqsave(&erst_lock).
Are there anything I'm missing?

Seiji


>-----Original Message-----
>From: Matthew Garrett [mailto:mjg@redhat.com]
>Sent: Tuesday, July 19, 2011 2:52 PM
>To: Seiji Aguchi
>Cc: kexec@lists.infradead.org; linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org; Eric W. Biederman; Vivek
>Goyal; KOSAKI Motohiro; Americo Wang; tony.luck@intel.com; Andrew Morton; Jarod Wilson; hpa@zytor.com; dzickus@redhat.com;
>dle-develop@lists.sourceforge.net; Satoru Moriya
>Subject: Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path
>
>On Tue, Jul 19, 2011 at 02:48:22PM -0400, Seiji Aguchi wrote:
>> >How is this safe? What happens if there's a pstore access in process
>> >when you hit this codepath?
>>
>> This will never happen because pstore_kmsg_dump_in_interrupt() is called after machine_crash_shutdown().
>>
>> Panicked cpu sends NMI to all other cpus in machine_crash_shutdown() and they stop.
>
>And how does that handle the case where we're halfway through a pstore
>access when the NMI arrives? ERST, at least, has a complex state
>machine. You can't guarantee what starting one transaction without
>completing one that was in process will do.
>
>--
>Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Don Zickus]

On Tue, Jul 19, 2011 at 03:02:12PM -0400, Vivek Goyal wrote:
> > Another interesting question is do we need to log anything in the kdump
> > path?  Isn't kdump generating the same info?  What added value do we get
> > over kdump?
> 
> I had the same question. The argument is that kdump can fail and they
> can not afford to not capture any info at all. So before kdump executes
> they want to save some state to NVRAM.
> 
> I am wondering that saving this info to NVRAM, can it be done early in
> second kernel? I guess we are not supposed to take any EFI services in
> second kernel so it might not be possible.

Actually the write to NVRAM wouldn't be so bad if ERST supported it.  It
would just be an equvialent to a memcpy.  Currently it uses a complicated
state machine to a persistent storage which complicates things.  As a
result I get nervous if ERST firmware issues would block us from executing
kdump.

Cheers,
Don

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Matthew Garrett]

On Tue, Jul 19, 2011 at 03:14:22PM -0400, Seiji Aguchi wrote:
> 
> >And how does that handle the case where we're halfway through a pstore
> >access when the NMI arrives? ERST, at least, has a complex state
> >machine. You can't guarantee what starting one transaction without
> >completing one that was in process will do.
> 
> As for ERST, write access is protected by raw_spin_trylock_irqsave(&erst_lock).
> Are there anything I'm missing?

If there's already locking involved, what benefit does removing the lock 
in the pstore code give? You'll just hang when you hit the erst code 
instead of the pstore code.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC][PATCH -mmotm 1/4] Add static function calls of pstore to kexec path

[Matthew Garrett]

On Tue, Jul 19, 2011 at 08:27:54PM +0100, Matthew Garrett wrote:
> On Tue, Jul 19, 2011 at 03:14:22PM -0400, Seiji Aguchi wrote:
> > 
> > >And how does that handle the case where we're halfway through a pstore
> > >access when the NMI arrives? ERST, at least, has a complex state
> > >machine. You can't guarantee what starting one transaction without
> > >completing one that was in process will do.
> > 
> > As for ERST, write access is protected by raw_spin_trylock_irqsave(&erst_lock).
> > Are there anything I'm missing?
> 
> If there's already locking involved, what benefit does removing the lock 
> in the pstore code give? You'll just hang when you hit the erst code 
> instead of the pstore code.

I'm sorry, you're right, this is a trylock so we won't hang in this 
case. We should probably document that in the pstore documentation to 
ensure that any future backends have the same behaviour.

-- 
Matthew Garrett | mjg59@srcf.ucam.org