Patchwork [7/7] sparc64: Add function graph tracer support.

login
register
mail settings
Submitter Frédéric Weisbecker
Date April 18, 2010, 3:31 p.m.
Message ID <20100418153121.GA5174@nowhere>
Download mbox | patch
Permalink /patch/50415/
State RFC
Delegated to: David Miller
Headers show

Comments

Frédéric Weisbecker - April 18, 2010, 3:31 p.m.
On Sat, Apr 17, 2010 at 02:38:37PM -0700, David Miller wrote:
> From: Frederic Weisbecker <fweisbec@gmail.com>
> Date: Sat, 17 Apr 2010 23:34:15 +0200
> 
> > I haven't started the watchdog nor perf, I guess NMI don't trigger
> > in other cases, right?
> 
> They do, for the NMI watchdog, every few seconds.
> 
> > For now, the only reentrancy I could find was irqs that interrupt
> > the tracing path. Which means no good clue there. That said I
> > have only logged recursivity on trace entry path, not yet
> > on return.
> > 
> > I'm disabling the protections on entry, just to narrow down
> > the recursivity place, in case it only happens on return.
> 
> No need to do so much work, when you hit this case simply
> disable tracing and dump_stack().  That way you'll see it
> clearly.


In fact it's quite hard to dump, because most of them I get
are irrelevant (irqs that do reentering the tracing path).
And after some time, dumps end up crashing.

All I could do is narrowing down the source, everything
happens well with this patch:




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

--
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
David Miller - April 18, 2010, 9:19 p.m.
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
David Miller - April 19, 2010, 7:56 a.m.
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
David Miller - April 19, 2010, 8:15 a.m.
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
Frédéric Weisbecker - April 19, 2010, 7:52 p.m.
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
David Miller - April 19, 2010, 7:56 p.m.
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
Frédéric Weisbecker - April 19, 2010, 8:37 p.m.
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
David Miller - April 20, 2010, 5:51 a.m.
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

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)