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

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

[Seiji Aguchi]

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 <seiji.aguchi@hds.com>

---
 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\n");
-	} 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. */
-- 
1.7.1

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

[Chen Gong]

于 2011/10/15 4:53, Seiji Aguchi 写道:
> 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, Seiji

I have a stupid question: since you have serialized the process procedure via 
smp_send_stop, why still using spin_lock_xxx? Maybe preempt_disable/enable is 
enough?

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

[Seiji Aguchi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1386 bytes --]

Hi,

Thank you for giving me a comment.

>I have a stupid question: since you have serialized the process procedure via
>smp_send_stop, why still using spin_lock_xxx? Maybe preempt_disable/enable is
>enough?


I added spin_lock_init() in panic path for sharing code with other triggers 
such as oops/reboot/emergency_restart because they still need spin_locks.

Do you suggest following code?

<snip>
If(!panic)
	spin_lock_irqsave();

     .
     .
If(!panic)
	spin_unlock_restore();
<snip>

I don't stick to current patch.
So I will resend a patch above if you request.

Regarding as preempt_disable/enable, we don't need to call them in panic path because they are
called at the beginning of panic().

	<snip>
	60 NORET_TYPE void panic(const char * fmt, ...)  
	61 {  
	62         static char buf[1024];  
	63         va_list args;  
	64         long i, i_next = 0;  
	65         int state = 0;  
	66   
	67         /*  
	68          * It's possible to come here directly from a panic-assertion and  
	69          * not have preempt disabled. Some functions called from here want  
	70          * preempt to be disabled. No point enabling it later though...  
	71          */  
	72         preempt_disable();
	<snip>

Seiji
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Chen Gong]

于 2011/10/17 22:10, Seiji Aguchi 写道:
> Hi,
>
> Thank you for giving me a comment.
>
>> I have a stupid question: since you have serialized the process procedure via
>> smp_send_stop, why still using spin_lock_xxx? Maybe preempt_disable/enable is
>> enough?
>
>
> I added spin_lock_init() in panic path for sharing code with other triggers
> such as oops/reboot/emergency_restart because they still need spin_locks.
>
> Do you suggest following code?
>
> <snip>
> If(!panic)
> 	spin_lock_irqsave();
>
>       .
>       .
> If(!panic)
> 	spin_unlock_restore();
> <snip>
>
> I don't stick to current patch.
> So I will resend a patch above if you request.

Yes, it looks more readable.

+	if (reason == KMSG_DUMP_PANIC)
+		raw_spin_lock_init(&logbuf_lock);

above codes look a little bit confused

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

[Don Zickus]

On Fri, Oct 14, 2011 at 04:53:05PM -0400, Seiji Aguchi wrote:
> 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.

I think I am ok with this, just some comments below.

> 
>  Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
> 
> ---
>  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\n");
> -	} 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);
>  }

My original problem was the NMI path was calling panic and thus causing
these locking problems.  With the change below, which allows the change
above, I believe the NMI case is covered (because this path is only called
by NMI during panic).  This makes my previous patch (accepted by Tony)
probably unnecessary if we go with this patch.

As for the need for using spin_lock_init vs just skipping the locking, I
don't have a preference, nor know what the correct usage is for these
things.  I suppose this patch does the proper thing.

>  
>  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);

Yes, this is what Seiji and I talked about previously based on Vivek's
suggestion.  This can only work on x86 if we pick up my 'using NMI in stop
cpu path' patch from last week.

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

Otherwise, there is no guarantee all the cpus stop and we can still end up
in data corruption depending on the backend being used.

> 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. */
> -- 

The just adds more lock busting based on the movement of kmsg_dump() in
the panic path.

There is still more lock busting to do in the various backends, but this
patch provides the necessary first steps.

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.

Acked-by: Don Zickus <dzickus@redhat.com>

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

[Luck, Tony]

> 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).

-Tony

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

[Don Zickus]

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

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

[Seiji Aguchi]

Hi,

I agree with Tony's concern.
But as Don told,  the best way to go proceed is fixing backend drivers step by step.

Seiji


>-----Original Message-----
>From: Don Zickus [mailto:dzickus@redhat.com]
>Sent: Monday, October 17, 2011 1:22 PM
>To: Luck, Tony
>Cc: Seiji Aguchi; linux-kernel@vger.kernel.org; Matthew Garrett; Vivek Goyal; Chen, Gong; Andrew Morton; Brown, Len; Huang,
>Ying; ak@linux.intel.com; hughd@chromium.org; mingo@elte.hu; jmorris@namei.org; a.p.zijlstra@chello.nl;
>namhyung@gmail.com; dle-develop@lists.sourceforge.net; 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

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

[Andrew Morton]

On Fri, 14 Oct 2011 16:53:05 -0400
Seiji Aguchi <seiji.aguchi@hds.com> wrote:

> 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.
>
> ...
>
> --- 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\n");
> -	} 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);
>  }

afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi().  Is
that always the case, and will it always be the case in the future?

I felt that the spin_trylock() approach was less horrid than this.  I
assume that the new approach will cause lockdep to go berzerk?

> --- 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. */

I suggest you do some research into bust_spinlocks() and how it has
changed over time.  I think that code used to fiddle with log levels
and once upon a time it might have fiddled with logbuf_lock.

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

[Don Zickus]

On Mon, Oct 17, 2011 at 04:47:15PM -0700, Andrew Morton wrote:
> On Fri, 14 Oct 2011 16:53:05 -0400
> > @@ -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);
> >  }
> 
> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi().  Is
> that always the case, and will it always be the case in the future?

I see your point.  For now yes.  The common case for which I think pstore
was designed for was the APEI/GHES case.  Normally when GHES hits an NMI
it stays at the APEI/GHES layer and either uses an irq_workqueue for
recoverable errors or panics on non-recoverable errors.

So currently the only time it reaches the pstore layer is in the panic
case.  Unfortunately, I can't vouch for all the backends that can hook
into pstore.

Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'?


> 
> I felt that the spin_trylock() approach was less horrid than this.  I
> assume that the new approach will cause lockdep to go berzerk?

Heh.  Good point.  That is probably a good test case.  Though finding a
working GHES implementation in the firmware isn't easy these days, making
it hard to test. :-/

Cheers,
Don

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

[Seiji Aguchi]

Hi,

>> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi().  Is
>> that always the case, and will it always be the case in the future?
>

Currently, when kernel is in nmi context and kmsg_dump() are called, its reason is always "KMSG_DUMP_PANIC".

>
>Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'?

I don't think BUG_ON() is needed.

If someone would like to log messages in the case of "in_nmi() && reason != KMSG_DUMP_PANIC",
he/she will add a new trigger as follows.

We can discuss implementation of kmsg_dump() at that time.
But I can't see any use case now.

	371 static notrace __kprobes void 
	372 unknown_nmi_error(unsigned char reason, struct pt_regs *regs) 
	373 {
	374         if (notify_die(DIE_NMIUNKNOWN, "nmi", regs, reason, 2, SIGINT) == 
	375                         NOTIFY_STOP) 
	376                 return; 
	377 #ifdef CONFIG_MCA 
	378         /* 
	379          * Might actually be able to figure out what the guilty party 
	380          * is: 
	381          */ 
	382         if (MCA_bus) { 
	383                 mca_handle_nmi(); 
	384                 return; 
	385         } 
	386 #endif 
	387         pr_emerg("Uhhuh. NMI received for unknown reason %02x on CPU %d.\n", 
	388                  reason, smp_processor_id()); 
	389  
	390         pr_emerg("Do you have a strange power saving mode enabled?\n"); 
	391         if (unknown_nmi_panic || panic_on_unrecovered_nmi) 
	392                 panic("NMI: Not continuing"); 
	393  
	394         pr_emerg("Dazed and confused, but trying to continue\n");
		    *kmsg_dump(NMI);*
	395 }

>>
>> I felt that the spin_trylock() approach was less horrid than this.  I
>> assume that the new approach will cause lockdep to go berzerk?
>
>Heh.  Good point.  That is probably a good test case.  Though finding a
>working GHES implementation in the firmware isn't easy these days, making
>it hard to test. :-/

Thank you for supporting my patch.
But I can resend another patch below in accordance with Chen's request.
We can avoid potential deplock issue with this patch.
And this is simpler than trylock approach.

> <snip>
> If(!panic)
> 	spin_lock_irqsave();
>
>       .
>       .
> If(!panic)
> 	spin_unlock_restore();
> <snip>

Seiji

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

[Don Zickus]

On Tue, Oct 18, 2011 at 10:52:22AM -0400, Seiji Aguchi wrote:
> Hi,
> 
> >> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi().  Is
> >> that always the case, and will it always be the case in the future?
> >
> 
> Currently, when kernel is in nmi context and kmsg_dump() are called, its reason is always "KMSG_DUMP_PANIC".
> 
> >
> >Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'?
> 
> I don't think BUG_ON() is needed.
> 
> If someone would like to log messages in the case of "in_nmi() && reason != KMSG_DUMP_PANIC",
> he/she will add a new trigger as follows.

The point I was trying to make with the BUG_ON is to catch future uses of
NMI and kmsg_dump that were implemented without understanding the
restriction we place (you have to be in a panic path for NMI to use
pstore/kmsg_dump).

That is part of the wider problem with kmsg_dump that Vivek talks about
with me, is that it is just a giant hook in the panic path with limited
auditing.  So we need to explicit set our expectations with BUG_ONs/WARNs
otherwise we might get bit later by them.

Cheers,
Don

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

[Seiji Aguchi]

Don,

>That is part of the wider problem with kmsg_dump that Vivek talks about
>with me, is that it is just a giant hook in the panic path with limited
>auditing.  So we need to explicit set our expectations with BUG_ONs/WARNs
>otherwise we might get bit later by them.

I found an issue while developing a patch v2.

We can't call BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)
because kmsg_dump(KMSG_DUMP_EMERG) is called in NMI context if "panic" kernel parameter is set.

I will keep doing research bust_spinlock() or vprintk() so that lockdep checking works and we can avoid any deadlocks.

Seiji

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

[Don Zickus]

On Thu, Oct 20, 2011 at 11:13:26AM -0400, Seiji Aguchi wrote:
> Don,
> 
> >That is part of the wider problem with kmsg_dump that Vivek talks about
> >with me, is that it is just a giant hook in the panic path with limited
> >auditing.  So we need to explicit set our expectations with BUG_ONs/WARNs
> >otherwise we might get bit later by them.
> 
> I found an issue while developing a patch v2.
> 
> We can't call BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)
> because kmsg_dump(KMSG_DUMP_EMERG) is called in NMI context if "panic" kernel parameter is set.

I am confused.  Did you mean 'panic' kernel parameter is _not_ set?

Actually, BUG_ON is probably overkill, perhaps a WARN_ON would be more
appropriate.

> 
> I will keep doing research bust_spinlock() or vprintk() so that lockdep checking works and we can avoid any deadlocks.

I wonder if it would be smarter to split your patch in half.  The part
where you move kmsg_dump to below stop_cpus is probably non-controversial.
That might make it sooner than the more controversial bust_spinlock
pieces.

Cheers,
Don

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

[Seiji Aguchi]

Hi,

>I am confused.  Did you mean 'panic' kernel parameter is _not_ set?
>

I meant that  if panic parameter _is_ set (like panic=1), 
emergency_restart() will be called in panic path.
And then, kmsg_dump(KMSG_DUMP_EMERG) will be called.

        <snip>
	60 NORET_TYPE void panic(const char * fmt, ...)  
	61 {
	.
	.
	123         if (panic_timeout != 0) { 
	124                 /* 
	125                  * This will not be a clean reboot, with everything 
	126                  * shutting down.  But if there is a chance of 
	127                  * rebooting the system it will be rebooted. 
	128                  */ 
	129                 emergency_restart(); 
	130         }
	<snip>

>Actually, BUG_ON is probably overkill, perhaps a WARN_ON would be more
>appropriate.

I don't think both BUG_ON and WARN_ON are appropriate.
kmsg_dump(KMSG_DUMP_EMERG) should be called successfully in panic path.

>I wonder if it would be smarter to split your patch in half.  The part
>where you move kmsg_dump to below stop_cpus is probably non-controversial.
>That might make it sooner than the more controversial bust_spinlock
>pieces.

Thank you for your suggestion. I will split my patch.

Seiji

>On Thu, Oct 20, 2011 at 11:13:26AM -0400, Seiji Aguchi wrote:
>> Don,
>>
>> >That is part of the wider problem with kmsg_dump that Vivek talks about
>> >with me, is that it is just a giant hook in the panic path with limited
>> >auditing.  So we need to explicit set our expectations with BUG_ONs/WARNs
>> >otherwise we might get bit later by them.
>>
>> I found an issue while developing a patch v2.
>>
>> We can't call BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)
>> because kmsg_dump(KMSG_DUMP_EMERG) is called in NMI context if "panic" kernel parameter is set.
>
>I am confused.  Did you mean 'panic' kernel parameter is _not_ set?
>
>Actually, BUG_ON is probably overkill, perhaps a WARN_ON would be more
>appropriate.
>
>>
>> I will keep doing research bust_spinlock() or vprintk() so that lockdep checking works and we can avoid any deadlocks.
>
>I wonder if it would be smarter to split your patch in half.  The part
>where you move kmsg_dump to below stop_cpus is probably non-controversial.
>That might make it sooner than the more controversial bust_spinlock
>pieces.
>
>Cheers,
>Don