linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()
@ 2012-04-09 17:20 Jason Wessel
  2012-04-09 18:34 ` Andi Kleen
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Wessel @ 2012-04-09 17:20 UTC (permalink / raw)
  To: akpm; +Cc: ak, linux-kernel

Commit 6e6f0a1f0 (panic: don't print redundant backtraces on oops)
causes a regression where no stack trace will be printed at all for
the case where kernel code calls panic() directly while not processing
an oops, and of course there are 100's of instances of this type of
call.

The original commit executed the check (!oops_in_progress), but this
will always be false because just before the dump_stack() there is a
call to bust_spinlocks(1), which does the following:

  void __attribute__((weak)) bust_spinlocks(int yes)
  {
	if (yes) {
		++oops_in_progress;

The proper way to resolve the problem that original commit tried to
solve is to avoid printing a stack dump from panic() when the either
of the following conditions is true:

  1) TAINT_DIE has been set (this is done by oops_end())
     This indicates and oops has already been printed.
  2) oops_in_progress > 1
     This guards against the rare case where panic() is invoked
     a second time, or in between oops_begin() and oops_end()

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: stable@vger.kernel.org # >= 3.3
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/panic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 80aed44..8ed89a1 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -97,7 +97,7 @@ void panic(const char *fmt, ...)
 	/*
 	 * Avoid nested stack-dumping if a panic occurs during oops processing
 	 */
-	if (!oops_in_progress)
+	if (!test_taint(TAINT_DIE) && oops_in_progress <= 1)
 		dump_stack();
 #endif
 
-- 
1.7.9.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()
  2012-04-09 17:20 [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic() Jason Wessel
@ 2012-04-09 18:34 ` Andi Kleen
  2012-04-09 19:08   ` Jason Wessel
  2012-04-10 19:19   ` Andrew Morton
  0 siblings, 2 replies; 6+ messages in thread
From: Andi Kleen @ 2012-04-09 18:34 UTC (permalink / raw)
  To: Jason Wessel; +Cc: akpm, linux-kernel


> The proper way to resolve the problem that original commit tried to
> solve is to avoid printing a stack dump from panic() when the either
> of the following conditions is true:
>
>    1) TAINT_DIE has been set (this is done by oops_end())
>       This indicates and oops has already been printed.
>    2) oops_in_progress>  1
>       This guards against the rare case where panic() is invoked
>       a second time, or in between oops_begin() and oops_end()

Oops. I guess can just revert it for now. Thanks for catching, Jason.

The proper solution is probably some variant of
http://git.kernel.org/?p=linux/kernel/git/ak/linux-mce-2.6.git;a=shortlog;h=refs/heads/mce/xpanic

Let the caller pass in the proper action instead of all these hacks.

-Andi



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()
  2012-04-09 18:34 ` Andi Kleen
@ 2012-04-09 19:08   ` Jason Wessel
  2012-04-09 19:36     ` Andi Kleen
  2012-04-10 19:19   ` Andrew Morton
  1 sibling, 1 reply; 6+ messages in thread
From: Jason Wessel @ 2012-04-09 19:08 UTC (permalink / raw)
  To: Andi Kleen; +Cc: akpm, linux-kernel

On 04/09/2012 01:34 PM, Andi Kleen wrote:
>> The proper way to resolve the problem that original commit tried to
>> solve is to avoid printing a stack dump from panic() when the either
>> of the following conditions is true:
>>
>>    1) TAINT_DIE has been set (this is done by oops_end())
>>       This indicates and oops has already been printed.
>>    2) oops_in_progress>  1
>>       This guards against the rare case where panic() is invoked
>>       a second time, or in between oops_begin() and oops_end()
> Oops. I guess can just revert it for now. Thanks for catching, Jason.
>
> The proper solution is probably some variant of
> http://git.kernel.org/?p=linux/kernel/git/ak/linux-mce-2.6.git;a=shortlog;h=refs/heads/mce/xpanic
>
> Let the caller pass in the proper action instead of all these hacks.

That is an interesting patch series, but I am not sure I agree about the caller propagating a flag to control what you see in panic.  I intentionally set CONFIG_DEBUG_VERBOSE=y for a reason, I want a stack trace if and when panic(), oops, BUG, etc... is ever called.  This might be a personal preference, but I do not wish to be searching on a string, to find out where the kernel execution terminated.  I want to see stack traces all the time, with the exception of the cases you pointed out in the original patch.

I had taken the time to instrument and the edge cases for the oops and panic() nesting.  I would agree that it is not useful to print further traces if the kernel has already processed an oops.  I also checked non-x86 archs to see that the original behavior was intact with the new version of the patch.  My opinion is that checking for the panic recursion is a necessary evil here, we just have to agree about the right approach.  :-)

Cheers,
Jason.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()
  2012-04-09 19:08   ` Jason Wessel
@ 2012-04-09 19:36     ` Andi Kleen
  0 siblings, 0 replies; 6+ messages in thread
From: Andi Kleen @ 2012-04-09 19:36 UTC (permalink / raw)
  To: Jason Wessel; +Cc: akpm, linux-kernel

> That is an interesting patch series, but I am not sure I agree about the caller propagating a flag to control what you see in panic.  I intentionally set CONFIG_DEBUG_VERBOSE=y for a reason, I want a stack trace if and when panic(), oops, BUG, etc... is ever called.  This might be a personal preference, but I do not wish to be searching on a string, to find out where the kernel execution terminated.  I want to see stack traces all the time, with the exception of the cases you pointed out in the original patch.

The idea was that callers who are calling panic not due to a programming
error (like no root, hardware error etc.) do not print the backtrace.
And only "assert like" panics would. 

This makes the screen less cluttered for user or hardware induced
panics.

Besides that it does a couple of related things, like set proper
timeouts.

-Andi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()
  2012-04-09 18:34 ` Andi Kleen
  2012-04-09 19:08   ` Jason Wessel
@ 2012-04-10 19:19   ` Andrew Morton
  2012-04-10 23:29     ` Andi Kleen
  1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-04-10 19:19 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Jason Wessel, linux-kernel

On Mon, 09 Apr 2012 11:34:48 -0700
Andi Kleen <ak@linux.intel.com> wrote:

> > The proper way to resolve the problem that original commit tried to
> > solve is to avoid printing a stack dump from panic() when the either
> > of the following conditions is true:
> >
> >    1) TAINT_DIE has been set (this is done by oops_end())
> >       This indicates and oops has already been printed.
> >    2) oops_in_progress>  1
> >       This guards against the rare case where panic() is invoked
> >       a second time, or in between oops_begin() and oops_end()
> 
> Oops. I guess can just revert it for now. Thanks for catching, Jason.

You're suggesting a revert of 6e6f0a1f0, but Jason is suggesting that
it be repaired.  Confused.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()
  2012-04-10 19:19   ` Andrew Morton
@ 2012-04-10 23:29     ` Andi Kleen
  0 siblings, 0 replies; 6+ messages in thread
From: Andi Kleen @ 2012-04-10 23:29 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Jason Wessel, linux-kernel

On Tue, Apr 10, 2012 at 12:19:49PM -0700, Andrew Morton wrote:
> On Mon, 09 Apr 2012 11:34:48 -0700
> Andi Kleen <ak@linux.intel.com> wrote:
> 
> > > The proper way to resolve the problem that original commit tried to
> > > solve is to avoid printing a stack dump from panic() when the either
> > > of the following conditions is true:
> > >
> > >    1) TAINT_DIE has been set (this is done by oops_end())
> > >       This indicates and oops has already been printed.
> > >    2) oops_in_progress>  1
> > >       This guards against the rare case where panic() is invoked
> > >       a second time, or in between oops_begin() and oops_end()
> > 
> > Oops. I guess can just revert it for now. Thanks for catching, Jason.
> 
> You're suggesting a revert of 6e6f0a1f0, but Jason is suggesting that
> it be repaired.  Confused.

Repairing is fine too for me.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-04-10 23:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-09 17:20 [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic() Jason Wessel
2012-04-09 18:34 ` Andi Kleen
2012-04-09 19:08   ` Jason Wessel
2012-04-09 19:36     ` Andi Kleen
2012-04-10 19:19   ` Andrew Morton
2012-04-10 23:29     ` Andi Kleen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).