| Message ID | 20100418153121.GA5174@nowhere |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
From: Frederic Weisbecker <fweisbec@gmail.com> Date: Sun, 18 Apr 2010 17:31:24 +0200 > That reminds me badly the problems with NMIs, and I saw > an NMI path in one of the dumps, so I guess you were > right after all as it looks like more an NMIs related problem > rather than a recursion. > > I thought if I won't enable the watchdog I wouldn't have NMIs > but I have some in /proc/interrupts Maybe this patch helps your case but I still get crashes even with NMIs disabled, since as I mentioned the other day I've already tested with them disabled. To turn them off, simply make arch/sparc/kernel/pcr.c:pcr_arch_init() immediately return 0 and you'll have no NMIs at all. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Frederic Weisbecker <fweisbec@gmail.com> Date: Sun, 18 Apr 2010 17:31:24 +0200 > All I could do is narrowing down the source, everything > happens well with this patch: > > > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c > index 9aed1a5..cfcb863 100644 > --- a/kernel/trace/trace_functions_graph.c > +++ b/kernel/trace/trace_functions_graph.c > @@ -287,7 +287,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace) > __trace_graph_return(tr, trace, flags, pc); > } > atomic_dec(&data->disabled); > + pause_graph_tracing(); > local_irq_restore(flags); > + unpause_graph_tracing(); > } > > void set_graph_array(struct trace_array *tr) So I was tooling around, doing some disassembly and seeing what these local_irq_*() paths look like... From one perspective, I can't see how we can justify not using the raw_*() variants in these critical tracer paths. With lockdep or the irqsoff tracer enabled, we can call various code paths that will recurse into the tracer, especially if the debugging checks trigger. And at this point we've decremented the ->disabled counter, so we will absolutely not be able to detect tracer recursion triggered by this local_irq_restore(). In fact, if we enter an error state wrt. trace_hardirqs_{on,off}() we will likely just trigger the error check there all over again when we re-enter the tracer and call local_irq_restore() once more. And this would explain the crazy recursion we get. Another idea is to decrement the ->disabled counter after the local_irq_restore(). Yes, we might lose IRQ handler traces which occur between the local_irq_restore() and the counter decrment, but we would also be completely immune to recursion problems. This was a great lead Frederic, it probably explains the bulk of our problems. Thanks for narrowing it down like this! -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Mon, 19 Apr 2010 00:56:13 -0700 (PDT) > Another idea is to decrement the ->disabled counter after the > local_irq_restore(). Yes, we might lose IRQ handler traces which > occur between the local_irq_restore() and the counter decrment, but we > would also be completely immune to recursion problems. So I just gave this a try and I still get the crashes I've been seeing with the non-graph function tracer, oh well :-) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 19, 2010 at 01:15:55AM -0700, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Mon, 19 Apr 2010 00:56:13 -0700 (PDT) > > > Another idea is to decrement the ->disabled counter after the > > local_irq_restore(). Yes, we might lose IRQ handler traces which > > occur between the local_irq_restore() and the counter decrment, but we > > would also be completely immune to recursion problems. > > So I just gave this a try and I still get the crashes I've > been seeing with the non-graph function tracer, oh well :-) Are you using the pause_graph_tracing() thing on the function tracer? If so it won't work as it only applies to the function graph tracer. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Frederic Weisbecker <fweisbec@gmail.com> Date: Mon, 19 Apr 2010 21:52:47 +0200 > On Mon, Apr 19, 2010 at 01:15:55AM -0700, David Miller wrote: >> From: David Miller <davem@davemloft.net> >> Date: Mon, 19 Apr 2010 00:56:13 -0700 (PDT) >> >> > Another idea is to decrement the ->disabled counter after the >> > local_irq_restore(). Yes, we might lose IRQ handler traces which >> > occur between the local_irq_restore() and the counter decrment, but we >> > would also be completely immune to recursion problems. >> >> So I just gave this a try and I still get the crashes I've >> been seeing with the non-graph function tracer, oh well :-) > > > Are you using the pause_graph_tracing() thing on the function > tracer? If so it won't work as it only applies to the function > graph tracer. No, I simply moved the atomic_dec() below the local_irq_restore() as stated in the first paragraph above :-) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 19, 2010 at 12:56:41PM -0700, David Miller wrote: > From: Frederic Weisbecker <fweisbec@gmail.com> > Date: Mon, 19 Apr 2010 21:52:47 +0200 > > > On Mon, Apr 19, 2010 at 01:15:55AM -0700, David Miller wrote: > >> From: David Miller <davem@davemloft.net> > >> Date: Mon, 19 Apr 2010 00:56:13 -0700 (PDT) > >> > >> > Another idea is to decrement the ->disabled counter after the > >> > local_irq_restore(). Yes, we might lose IRQ handler traces which > >> > occur between the local_irq_restore() and the counter decrment, but we > >> > would also be completely immune to recursion problems. > >> > >> So I just gave this a try and I still get the crashes I've > >> been seeing with the non-graph function tracer, oh well :-) > > > > > > Are you using the pause_graph_tracing() thing on the function > > tracer? If so it won't work as it only applies to the function > > graph tracer. > > No, I simply moved the atomic_dec() below the local_irq_restore() > as stated in the first paragraph above :-) Ah right, read too fast :) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Frederic, I think I got it. When CONFIG_TRACE_IRQFLAGS is enabled, we can recurse forever in IRQ traps, and this is exacerbated by the slowness of tracing as it makes it more likely we'll hit the small sequence of instructions where we erroneously enable irqs in the trap return path. I'm doing my stress tests right now, but if you want to try it out in parallel, comment out the: wrpr %l4, %pil enclosed in CONFIG_TRACE_IRQFLAGS in arch/sparc/kernel/rtrap_64.S The issue here is that we normally restore the trap time %pil further in the trap return sequence, but this is much beyond the point where we can still invoke C functions properly. Which means that we can't call trace_hardirqs_on() there. So I was trying to do it early. But this also means we restore the %pil before we hard disable all interrupts by clearing PSTATE_IE in the %pstate register. And this is what creates the window of instructions where every IRQ return can erroneously take another IRQ trap. I have to think about why I might have done things this way to make sure I fix this right. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c index 9aed1a5..cfcb863 100644 --- a/kernel/trace/trace_functions_graph.c +++ b/kernel/trace/trace_functions_graph.c @@ -287,7 +287,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace) __trace_graph_return(tr, trace, flags, pc); } atomic_dec(&data->disabled); + pause_graph_tracing(); local_irq_restore(flags); + unpause_graph_tracing(); } void set_graph_array(struct trace_array *tr)