diff mbox

ARM64: smp: Silence suspicious RCU usage with ipi tracepoints

Message ID 1435176858-9568-1-git-send-email-sboyd@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Stephen Boyd June 24, 2015, 8:14 p.m. UTC
John Stultz reported an RCU splat on ARM with ipi trace events
enabled. It looks like the same problem exists on ARM64.

At this point in the IPI handling path we haven't called
irq_enter() yet, so RCU doesn't know that we're about to exit
idle and properly warns that we're using RCU from an idle CPU.
Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
that RCU is informed about our exit from idle.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Fixes: 45ed695ac10a "ARM64: add IPI tracepoints"
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

I haven't confirmed this, but it looks an awful lot like the same
problem exists on ARM64.

 arch/arm64/kernel/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Paul E. McKenney June 24, 2015, 8:20 p.m. UTC | #1
On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
> John Stultz reported an RCU splat on ARM with ipi trace events
> enabled. It looks like the same problem exists on ARM64.
> 
> At this point in the IPI handling path we haven't called
> irq_enter() yet, so RCU doesn't know that we're about to exit
> idle and properly warns that we're using RCU from an idle CPU.
> Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
> that RCU is informed about our exit from idle.
> 
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Fixes: 45ed695ac10a "ARM64: add IPI tracepoints"
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
> 
> I haven't confirmed this, but it looks an awful lot like the same
> problem exists on ARM64.
> 
>  arch/arm64/kernel/smp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 4b2121bd7f9c..a1883bfdd9d6 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -693,7 +693,7 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>  	struct pt_regs *old_regs = set_irq_regs(regs);
> 
>  	if ((unsigned)ipinr < NR_IPI) {
> -		trace_ipi_entry(ipi_types[ipinr]);
> +		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
>  		__inc_irq_stat(cpu, ipi_irqs[ipinr]);
>  	}
> 
> @@ -736,7 +736,7 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>  	}
> 
>  	if ((unsigned)ipinr < NR_IPI)
> -		trace_ipi_exit(ipi_types[ipinr]);
> +		trace_ipi_exit_rcuidle(ipi_types[ipinr]);
>  	set_irq_regs(old_regs);
>  }
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Peter Zijlstra June 24, 2015, 9:29 p.m. UTC | #2
On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
> John Stultz reported an RCU splat on ARM with ipi trace events
> enabled. It looks like the same problem exists on ARM64.
> 
> At this point in the IPI handling path we haven't called
> irq_enter() yet, so RCU doesn't know that we're about to exit
> idle and properly warns that we're using RCU from an idle CPU.
> Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
> that RCU is informed about our exit from idle.

I have a problem with $subject. It says 'silence', whereas afaict this
fixes an actual bug, so it should be 'fixes'.
Stephen Boyd June 24, 2015, 9:59 p.m. UTC | #3
On 06/24/2015 02:29 PM, Peter Zijlstra wrote:
> On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
>> John Stultz reported an RCU splat on ARM with ipi trace events
>> enabled. It looks like the same problem exists on ARM64.
>>
>> At this point in the IPI handling path we haven't called
>> irq_enter() yet, so RCU doesn't know that we're about to exit
>> idle and properly warns that we're using RCU from an idle CPU.
>> Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
>> that RCU is informed about our exit from idle.
> I have a problem with $subject. It says 'silence', whereas afaict this
> fixes an actual bug, so it should be 'fixes'.

Fair enough. I can resend or whoever applies this patch can
s/Silence/Fix/ on the $subject.
Steven Rostedt June 25, 2015, 1:01 a.m. UTC | #4
On Wed, 24 Jun 2015 23:29:30 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
> > John Stultz reported an RCU splat on ARM with ipi trace events
> > enabled. It looks like the same problem exists on ARM64.
> > 
> > At this point in the IPI handling path we haven't called
> > irq_enter() yet, so RCU doesn't know that we're about to exit
> > idle and properly warns that we're using RCU from an idle CPU.
> > Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
> > that RCU is informed about our exit from idle.
> 
> I have a problem with $subject. It says 'silence', whereas afaict this
> fixes an actual bug, so it should be 'fixes'.

Agreed, otherwise Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve
Catalin Marinas June 25, 2015, 1:24 p.m. UTC | #5
On Wed, Jun 24, 2015 at 02:59:30PM -0700, Stephen Boyd wrote:
> On 06/24/2015 02:29 PM, Peter Zijlstra wrote:
> > On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
> >> John Stultz reported an RCU splat on ARM with ipi trace events
> >> enabled. It looks like the same problem exists on ARM64.
> >>
> >> At this point in the IPI handling path we haven't called
> >> irq_enter() yet, so RCU doesn't know that we're about to exit
> >> idle and properly warns that we're using RCU from an idle CPU.
> >> Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
> >> that RCU is informed about our exit from idle.
> > I have a problem with $subject. It says 'silence', whereas afaict this
> > fixes an actual bug, so it should be 'fixes'.
> 
> Fair enough. I can resend or whoever applies this patch can
> s/Silence/Fix/ on the $subject.

I'll apply the patch and change the subject as well.

Thanks.
Russell King - ARM Linux June 29, 2015, 10:02 a.m. UTC | #6
On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
> John Stultz reported an RCU splat on ARM with ipi trace events
> enabled. It looks like the same problem exists on ARM64.
> 
> At this point in the IPI handling path we haven't called
> irq_enter() yet, so RCU doesn't know that we're about to exit
> idle and properly warns that we're using RCU from an idle CPU.
> Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
> that RCU is informed about our exit from idle.
> 
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Fixes: 45ed695ac10a "ARM64: add IPI tracepoints"
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

I can find no trace of your equivalent ARM patch in my mailbox, but my
comment is as relevant to that message as it is to this one.

The Fixes: line has a standard format, which is not the formatting above.
Please see Documentation/SubmittingPatches section 2.  Thanks.

I've fixed it when applying the ARM version of this patch.  However, I'm
left wondering whether the lack of update to change the "Silence" to "Fix"
which I _stumbled_ over later in this thread is laziness on your part -
had it not been for the malformed Fixes: line, I would not have known to
update that.

We all gets lots of email every day, expecting people to remember some
comment in the dim and distant past made to a similar but different patch
is not really on.  Please ensure that you update your patches in future.
Catalin Marinas June 29, 2015, 11:23 a.m. UTC | #7
On Mon, Jun 29, 2015 at 11:02:51AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 24, 2015 at 01:14:18PM -0700, Stephen Boyd wrote:
> > John Stultz reported an RCU splat on ARM with ipi trace events
> > enabled. It looks like the same problem exists on ARM64.
> > 
> > At this point in the IPI handling path we haven't called
> > irq_enter() yet, so RCU doesn't know that we're about to exit
> > idle and properly warns that we're using RCU from an idle CPU.
> > Use trace_ipi_entry_rcuidle() instead of trace_ipi_entry() so
> > that RCU is informed about our exit from idle.
> > 
> > Cc: John Stultz <john.stultz@linaro.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Fixes: 45ed695ac10a "ARM64: add IPI tracepoints"
[...]
> The Fixes: line has a standard format, which is not the formatting above.
> Please see Documentation/SubmittingPatches section 2.  Thanks.

I re-wrote it in this instance. What's not clear to me is whether cc:
stable is still necessary with a Fixes: tag (I add it just in case).

Anyway, to simplify the process, I came up with a .gitconfig alias:

[alias]
        show-fixes = !git show --pretty=format:\"Fixes: %h (\\\"%s\\\")\nCc: <stable@vger.kernel.org> "#" $(git describe --match="v*" --contains $1 | cut -d - -f -1)+\n\n%b\" $1 "#"

And another for .vimrc (to replace a Fixes: line):

command! GitFixes
	\ :.!read tagname commit; git show-fixes $commit 2> /dev/null | head -n 2
diff mbox

Patch

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 4b2121bd7f9c..a1883bfdd9d6 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -693,7 +693,7 @@  void handle_IPI(int ipinr, struct pt_regs *regs)
 	struct pt_regs *old_regs = set_irq_regs(regs);
 
 	if ((unsigned)ipinr < NR_IPI) {
-		trace_ipi_entry(ipi_types[ipinr]);
+		trace_ipi_entry_rcuidle(ipi_types[ipinr]);
 		__inc_irq_stat(cpu, ipi_irqs[ipinr]);
 	}
 
@@ -736,7 +736,7 @@  void handle_IPI(int ipinr, struct pt_regs *regs)
 	}
 
 	if ((unsigned)ipinr < NR_IPI)
-		trace_ipi_exit(ipi_types[ipinr]);
+		trace_ipi_exit_rcuidle(ipi_types[ipinr]);
 	set_irq_regs(old_regs);
 }