[PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

Peter, I've respun the local_irq_*_nmi() interface addition, sparc64
implementation, and trace/sched_clock annotations against Linus's
tree.

I've tested all of the tracers in various different configurations on
sparc64 and I haven't triggered any of the bug checks or crashes yet
so it should be good.

It can also be pulled from:

	master.kernel.org:/pub/scm/linux/kernel/git/davem/trace-2.6.git

And it would be nice to see this upstream soon so I can build upon
it :-)

You'll need to add the perf locking fix annotation to the -tip tree
as per you initial patch set, that didn't apply to mainline so I
didn't put it in here.

Thanks!

--------------------

The following changes since commit 0eddb519b9127c73d53db4bf3ec1d45b13f844d1:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../roland/infiniband

are available in the git repository at:

  master.kernel.org:/pub/scm/linux/kernel/git/davem/trace-2.6.git master

David S. Miller (2):
      sparc64: Implement local_irq_save_nmi() override.
      tracing: Use local_irq_{save,restore}_nmi() in tracers.

Peter Zijlstra (2):
      kernel: local_irq_{save,restore}_nmi()
      sched: Use local_irq_save_nmi() in cpu_clock()

 arch/sparc/include/asm/irqflags_64.h |   29 +++++++++++++++++++
 include/linux/irqflags.h             |   51 ++++++++++++++++++++++++++++++++--
 kernel/lockdep.c                     |    7 ++++
 kernel/sched_clock.c                 |    4 +-
 kernel/trace/ftrace.c                |    8 ++--
 kernel/trace/trace_functions.c       |    8 ++--
 kernel/trace/trace_functions_graph.c |    8 ++--
 kernel/trace/trace_irqsoff.c         |    8 +++++
 kernel/trace/trace_sched_wakeup.c    |    4 +-
 kernel/trace/trace_stack.c           |    4 +-
 10 files changed, 110 insertions(+), 21 deletions(-)

[PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

Peter, I've respun the local_irq_*_nmi() interface addition, sparc64
implementation, and trace/sched_clock annotations against Linus's
tree.

I've tested all of the tracers in various different configurations on
sparc64 and I haven't triggered any of the bug checks or crashes yet
so it should be good.

It can also be pulled from:

	master.kernel.org:/pub/scm/linux/kernel/git/davem/trace-2.6.git

And it would be nice to see this upstream soon so I can build upon
it :-)

You'll need to add the perf locking fix annotation to the -tip tree
as per you initial patch set, that didn't apply to mainline so I
didn't put it in here.

Thanks!

--------------------

The following changes since commit 0eddb519b9127c73d53db4bf3ec1d45b13f844d1:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../roland/infiniband

are available in the git repository at:

  master.kernel.org:/pub/scm/linux/kernel/git/davem/trace-2.6.git master

David S. Miller (2):
      sparc64: Implement local_irq_save_nmi() override.
      tracing: Use local_irq_{save,restore}_nmi() in tracers.

Peter Zijlstra (2):
      kernel: local_irq_{save,restore}_nmi()
      sched: Use local_irq_save_nmi() in cpu_clock()

 arch/sparc/include/asm/irqflags_64.h |   29 +++++++++++++++++++
 include/linux/irqflags.h             |   51 ++++++++++++++++++++++++++++++++--
 kernel/lockdep.c                     |    7 ++++
 kernel/sched_clock.c                 |    4 +-
 kernel/trace/ftrace.c                |    8 ++--
 kernel/trace/trace_functions.c       |    8 ++--
 kernel/trace/trace_functions_graph.c |    8 ++--
 kernel/trace/trace_irqsoff.c         |    8 +++++
 kernel/trace/trace_sched_wakeup.c    |    4 +-
 kernel/trace/trace_stack.c           |    4 +-
 10 files changed, 110 insertions(+), 21 deletions(-)

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

I'm rescinding these patches for now, for two reasons:

1) The NMI annotations necessary to get everything working really gets
   out of control.

2) I think I found a cheap way to do this in sparc64 specific code.

Thanks!

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

I'm rescinding these patches for now, for two reasons:

1) The NMI annotations necessary to get everything working really gets
   out of control.

2) I think I found a cheap way to do this in sparc64 specific code.

Thanks!

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[Peter Zijlstra]

On Mon, 2010-04-12 at 22:08 -0700, David Miller wrote:
> I'm rescinding these patches for now, for two reasons:
> 
> 1) The NMI annotations necessary to get everything working really gets
>    out of control.
> 
> 2) I think I found a cheap way to do this in sparc64 specific code.

Ah, ok..

Care to enlighten the curious amongst us as to what exactly you're doing
in the sparc code?

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[Peter Zijlstra]

On Mon, 2010-04-12 at 22:08 -0700, David Miller wrote:
> I'm rescinding these patches for now, for two reasons:
> 
> 1) The NMI annotations necessary to get everything working really gets
>    out of control.
> 
> 2) I think I found a cheap way to do this in sparc64 specific code.

Ah, ok..

Care to enlighten the curious amongst us as to what exactly you're doing
in the sparc code?

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 13 Apr 2010 09:48:58 +0200

> Care to enlighten the curious amongst us as to what exactly you're
> doing in the sparc code?

From 0c25e9e6cbe7b233bb91d14d0e2c258bf8e6ec83 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@davemloft.net>
Date: Mon, 12 Apr 2010 22:21:52 -0700
Subject: [PATCH 2/2] sparc64: Adjust __raw_local_irq_save() to cooperate in NMIs.

If we are in an NMI then doing a plain raw_local_irq_disable() will
write PIL_NORMAL_MAX into %pil, which is lower than PIL_NMI, and thus
we'll re-enable NMIs and recurse.

Doing a simple:

	%pil = %pil | PIL_NORMAL_MAX

does what we want, if we're already at PIL_NMI (15) we leave it at
that setting, else we set it to PIL_NORMAL_MAX (14).

This should get the function tracer working on sparc64.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/include/asm/irqflags_64.h |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/include/asm/irqflags_64.h b/arch/sparc/include/asm/irqflags_64.h
index 8b49bf9..a16e94c 100644
--- a/arch/sparc/include/asm/irqflags_64.h
+++ b/arch/sparc/include/asm/irqflags_64.h
@@ -76,9 +76,19 @@ static inline int raw_irqs_disabled(void)
  */
 static inline unsigned long __raw_local_irq_save(void)
 {
-	unsigned long flags = __raw_local_save_flags();
+	unsigned long flags, tmp;
 
-	raw_local_irq_disable();
+	/* Disable interrupts to PIL_NORMAL_MAX unless we already
+	 * are using PIL_NMI, in which case PIL_NMI is retained.
+	 */
+	__asm__ __volatile__(
+		"rdpr	%%pil, %0\n\t"
+		"or	%0, %2, %1\n\t"
+		"wrpr	%1, 0x0, %%pil"
+		: "=r" (flags), "=r" (tmp)
+		: "i" (PIL_NORMAL_MAX)
+		: "memory"
+	);
 
 	return flags;
 }
-- 
1.7.0.4

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 13 Apr 2010 09:48:58 +0200

> Care to enlighten the curious amongst us as to what exactly you're
> doing in the sparc code?

>From 0c25e9e6cbe7b233bb91d14d0e2c258bf8e6ec83 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@davemloft.net>
Date: Mon, 12 Apr 2010 22:21:52 -0700
Subject: [PATCH 2/2] sparc64: Adjust __raw_local_irq_save() to cooperate in NMIs.

If we are in an NMI then doing a plain raw_local_irq_disable() will
write PIL_NORMAL_MAX into %pil, which is lower than PIL_NMI, and thus
we'll re-enable NMIs and recurse.

Doing a simple:

	%pil = %pil | PIL_NORMAL_MAX

does what we want, if we're already at PIL_NMI (15) we leave it at
that setting, else we set it to PIL_NORMAL_MAX (14).

This should get the function tracer working on sparc64.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/include/asm/irqflags_64.h |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/include/asm/irqflags_64.h b/arch/sparc/include/asm/irqflags_64.h
index 8b49bf9..a16e94c 100644
--- a/arch/sparc/include/asm/irqflags_64.h
+++ b/arch/sparc/include/asm/irqflags_64.h
@@ -76,9 +76,19 @@ static inline int raw_irqs_disabled(void)
  */
 static inline unsigned long __raw_local_irq_save(void)
 {
-	unsigned long flags = __raw_local_save_flags();
+	unsigned long flags, tmp;
 
-	raw_local_irq_disable();
+	/* Disable interrupts to PIL_NORMAL_MAX unless we already
+	 * are using PIL_NMI, in which case PIL_NMI is retained.
+	 */
+	__asm__ __volatile__(
+		"rdpr	%%pil, %0\n\t"
+		"or	%0, %2, %1\n\t"
+		"wrpr	%1, 0x0, %%pil"
+		: "=r" (flags), "=r" (tmp)
+		: "i" (PIL_NORMAL_MAX)
+		: "memory"
+	);
 
 	return flags;
 }
-- 
1.7.0.4

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 13 Apr 2010 09:48:58 +0200

> Care to enlighten the curious amongst us as to what exactly you're
> doing in the sparc code?

From 0c25e9e6cbe7b233bb91d14d0e2c258bf8e6ec83 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@davemloft.net>
Date: Mon, 12 Apr 2010 22:21:52 -0700
Subject: [PATCH 2/2] sparc64: Adjust __raw_local_irq_save() to cooperate in NMIs.

If we are in an NMI then doing a plain raw_local_irq_disable() will
write PIL_NORMAL_MAX into %pil, which is lower than PIL_NMI, and thus
we'll re-enable NMIs and recurse.

Doing a simple:

	%pil = %pil | PIL_NORMAL_MAX

does what we want, if we're already at PIL_NMI (15) we leave it at
that setting, else we set it to PIL_NORMAL_MAX (14).

This should get the function tracer working on sparc64.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/include/asm/irqflags_64.h |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/include/asm/irqflags_64.h b/arch/sparc/include/asm/irqflags_64.h
index 8b49bf9..a16e94c 100644
--- a/arch/sparc/include/asm/irqflags_64.h
+++ b/arch/sparc/include/asm/irqflags_64.h
@@ -76,9 +76,19 @@ static inline int raw_irqs_disabled(void)
  */
 static inline unsigned long __raw_local_irq_save(void)
 {
-	unsigned long flags = __raw_local_save_flags();
+	unsigned long flags, tmp;
 
-	raw_local_irq_disable();
+	/* Disable interrupts to PIL_NORMAL_MAX unless we already
+	 * are using PIL_NMI, in which case PIL_NMI is retained.
+	 */
+	__asm__ __volatile__(
+		"rdpr	%%pil, %0\n\t"
+		"or	%0, %2, %1\n\t"
+		"wrpr	%1, 0x0, %%pil"
+		: "=r" (flags), "=r" (tmp)
+		: "i" (PIL_NORMAL_MAX)
+		: "memory"
+	);
 
 	return flags;
 }
-- 
1.7.0.4

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[Peter Zijlstra]

On Tue, 2010-04-13 at 00:56 -0700, David Miller wrote:

> If we are in an NMI then doing a plain raw_local_irq_disable() will
> write PIL_NORMAL_MAX into %pil, which is lower than PIL_NMI, and thus
> we'll re-enable NMIs and recurse.
> 
> Doing a simple:
> 
> 	%pil = %pil | PIL_NORMAL_MAX
> 
> does what we want, if we're already at PIL_NMI (15) we leave it at
> that setting, else we set it to PIL_NORMAL_MAX (14).

Ah indeed, and without a conditional, very nice! 

It does rely on the exact values of the PIL_levels, it might make sense
to note that in the comment, something like:

  * Assumes: PIL_NMI | PIL_NORMAL_MAX == PIL_NMI.

Hmm, it also assumes %pil is never anything other than 0,
PIL_NORMAL_MAX, PIL_NMI, because if:

  (%pil & 1) && (%pil != PIL_NMI)

then you'll end up disabling NMIs. Could something like that ever
happen?

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[Peter Zijlstra]

On Tue, 2010-04-13 at 00:56 -0700, David Miller wrote:

> If we are in an NMI then doing a plain raw_local_irq_disable() will
> write PIL_NORMAL_MAX into %pil, which is lower than PIL_NMI, and thus
> we'll re-enable NMIs and recurse.
> 
> Doing a simple:
> 
> 	%pil = %pil | PIL_NORMAL_MAX
> 
> does what we want, if we're already at PIL_NMI (15) we leave it at
> that setting, else we set it to PIL_NORMAL_MAX (14).

Ah indeed, and without a conditional, very nice! 

It does rely on the exact values of the PIL_levels, it might make sense
to note that in the comment, something like:

  * Assumes: PIL_NMI | PIL_NORMAL_MAX = PIL_NMI.

Hmm, it also assumes %pil is never anything other than 0,
PIL_NORMAL_MAX, PIL_NMI, because if:

  (%pil & 1) && (%pil != PIL_NMI)

then you'll end up disabling NMIs. Could something like that ever
happen?

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 13 Apr 2010 10:37:14 +0200

> Hmm, it also assumes %pil is never anything other than 0,
> PIL_NORMAL_MAX, PIL_NMI, because if:
> 
>   (%pil & 1) && (%pil != PIL_NMI)
> 
> then you'll end up disabling NMIs. Could something like that ever
> happen?

The only values we ever program into the %pil are 0, PIL_NORMAL_MAX
and PIL_NMI

Since PIL_NMI is the largest %pil value and all bits are set in it
(0xf), it doesn't matter what PIL_NORMAL_MAX actually is.

Yes this all deserves a comment, I'll add one, thanks for pointing
that out.

Re: [PATCH 0/4]: Respin local_irq_*_nmi() stuff.

[David Miller]

From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 13 Apr 2010 10:37:14 +0200

> Hmm, it also assumes %pil is never anything other than 0,
> PIL_NORMAL_MAX, PIL_NMI, because if:
> 
>   (%pil & 1) && (%pil != PIL_NMI)
> 
> then you'll end up disabling NMIs. Could something like that ever
> happen?

The only values we ever program into the %pil are 0, PIL_NORMAL_MAX
and PIL_NMI

Since PIL_NMI is the largest %pil value and all bits are set in it
(0xf), it doesn't matter what PIL_NORMAL_MAX actually is.

Yes this all deserves a comment, I'll add one, thanks for pointing
that out.