[PATCH] Inhibit NMI watchdog when Alt-SysRq-T operation is underway.

[PATCH] Inhibit NMI watchdog when Alt-SysRq-T operation is underway.

[Konrad Rzeszutek]

On large memory configuration with not so fast CPUs the NMI watchdog 
is triggered when memory addresses are being gathered and printed. 
The code paths for Alt-SysRq-t are sprinkled with touch_nmi_watchdog 
in various places but not in this routine (or in the loop that utilizes
this function). The patch has been tested for regression on large CPU+memory 
configuration (128 logical CPUs + 224 GB) and 1,2,4,16-CPU sockets with various 
memory sizes (1,2,4,6,20). 

Re: [PATCH] Inhibit NMI watchdog when Alt-SysRq-T operation is underway.

[darnok]

On Mon, Jul 09, 2007 at 10:02:42AM -0400, Konrad Rzeszutek wrote:
> On large memory configuration with not so fast CPUs the NMI watchdog 
> is triggered when memory addresses are being gathered and printed. 
> The code paths for Alt-SysRq-t are sprinkled with touch_nmi_watchdog 
> in various places but not in this routine (or in the loop that utilizes
> this function). The patch has been tested for regression on large CPU+memory 
> configuration (128 logical CPUs + 224 GB) and 1,2,4,16-CPU sockets with various 
> memory sizes (1,2,4,6,20). 

And the patch:

diff --git a/arch/x86_64/kernel/traps.c b/arch/x86_64/kernel/traps.c
index aac1c0b..b6e7d67 100644
--- a/arch/x86_64/kernel/traps.c
+++ b/arch/x86_64/kernel/traps.c
@@ -330,6 +330,10 @@ static int print_trace_stack(void *data,
 
 static void print_trace_address(void *data, unsigned long addr)
 {
+	static int i = 0;		
+	if (i && ((i % 8) == 0)) 
+		touch_nmi_watchdog();
+	i++;
 	printk_address(addr);
 }
 

Re: [PATCH] Inhibit NMI watchdog when Alt-SysRq-T operation is underway.

[Andrew Morton]

On Mon, 9 Jul 2007 11:53:02 -0400
darnok@68k.org wrote:

>  static void print_trace_address(void *data, unsigned long addr)
>  {
> +	static int i = 0;		
> +	if (i && ((i % 8) == 0)) 
> +		touch_nmi_watchdog();
> +	i++;
>  	printk_address(addr);
>  }

I doubt if the "% 8" thing is really needed?  printk_address() is pretty
slow and touch_nmi_watchdog is _reasonably_ fast.  It could be made heaps
faster by:

From: Andrew Morton <akpm@linux-foundation.org>

Avoid dirtying remote cpu's memory if it already has the correct value.

Cc: Andi Kleen <ak@suse.de>
Cc: Konrad Rzeszutek <konrad@darnok.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/i386/kernel/nmi.c |    8 +++++---
 x86_64/kernel/nmi.c    |    0 
 2 files changed, 5 insertions(+), 3 deletions(-)

diff -puN arch/i386/kernel/nmi.c~i386-speedup-touch_nmi_watchdog arch/i386/kernel/nmi.c
--- a/arch/i386/kernel/nmi.c~i386-speedup-touch_nmi_watchdog
+++ a/arch/i386/kernel/nmi.c
@@ -298,7 +298,7 @@ static unsigned int
 	last_irq_sums [NR_CPUS],
 	alert_counter [NR_CPUS];
 
-void touch_nmi_watchdog (void)
+void touch_nmi_watchdog(void)
 {
 	if (nmi_watchdog > 0) {
 		unsigned cpu;
@@ -307,8 +307,10 @@ void touch_nmi_watchdog (void)
 		 * Just reset the alert counters, (other CPUs might be
 		 * spinning on locks we hold):
 		 */
-		for_each_present_cpu (cpu)
-			alert_counter[cpu] = 0;
+		for_each_present_cpu(cpu) {
+			if (alert_counter[cpu])
+				alert_counter[cpu] = 0;
+		}
 	}
 
 	/*

So I'd be inclined to simplify your patch to a bare

From: Konrad Rzeszutek <konrad@darnok.org>

On large memory configuration with not so fast CPUs the NMI watchdog is
triggered when memory addresses are being gathered and printed.  The code
paths for Alt-SysRq-t are sprinkled with touch_nmi_watchdog in various
places but not in this routine (or in the loop that utilizes this
function).  The patch has been tested for regression on large CPU+memory
configuration (128 logical CPUs + 224 GB) and 1,2,4,16-CPU sockets with
various memory sizes (1,2,4,6,20).

Cc: Andi Kleen <ak@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/x86_64/kernel/traps.c |    1 +
 1 files changed, 1 insertion(+)

diff -puN arch/x86_64/kernel/traps.c~inhibit-nmi-watchdog-when-alt-sysrq-t-operation-is-underway arch/x86_64/kernel/traps.c
--- a/arch/x86_64/kernel/traps.c~inhibit-nmi-watchdog-when-alt-sysrq-t-operation-is-underway
+++ a/arch/x86_64/kernel/traps.c
@@ -397,6 +397,7 @@ static int print_trace_stack(void *data,
 
 static void print_trace_address(void *data, unsigned long addr)
 {
+	touch_nmi_watchdog();
 	printk_address(addr);
 }
 
_


OK?

Re: [PATCH] Inhibit NMI watchdog when Alt-SysRq-T operation is underway.

[Konrad Rzeszutek]

Hey Andrew,

I tested your patch along with mine and found two things out:

 1). Missing this patch (for i386 platform)

diff --git a/arch/i386/kernel/traps.c b/arch/i386/kernel/traps.c
index 90da057..9f3a7ff 100644
--- a/arch/i386/kernel/traps.c
+++ b/arch/i386/kernel/traps.c
@@ -207,6 +207,7 @@ static void print_trace_address(void *da
 {
 	printk("%s [<%08lx>] ", (char *)data, addr);
 	print_symbol("%s\n", addr);
+	touch_nmi_watchdog();
 }
 
 static struct stacktrace_ops print_trace_ops = {


  2). If I run Alt-SysRq-t about 5000 times in a loop, the slow down 
      with this change is about 5%. Is this a big issue? (This was
      testing both i686 and x86_64).

On Fri, Jul 13, 2007 at 04:45:04PM -0700, Andrew Morton wrote:
> On Mon, 9 Jul 2007 11:53:02 -0400
> darnok@68k.org wrote:
> 
> >  static void print_trace_address(void *data, unsigned long addr)
> >  {
> > +	static int i = 0;		
> > +	if (i && ((i % 8) == 0)) 
> > +		touch_nmi_watchdog();
> > +	i++;
> >  	printk_address(addr);
> >  }
> 
> I doubt if the "% 8" thing is really needed?  printk_address() is pretty
> slow and touch_nmi_watchdog is _reasonably_ fast.  It could be made heaps
> faster by:
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> Avoid dirtying remote cpu's memory if it already has the correct value.
> 
> Cc: Andi Kleen <ak@suse.de>
> Cc: Konrad Rzeszutek <konrad@darnok.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  arch/i386/kernel/nmi.c |    8 +++++---
>  x86_64/kernel/nmi.c    |    0 
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff -puN arch/i386/kernel/nmi.c~i386-speedup-touch_nmi_watchdog arch/i386/kernel/nmi.c
> --- a/arch/i386/kernel/nmi.c~i386-speedup-touch_nmi_watchdog
> +++ a/arch/i386/kernel/nmi.c
> @@ -298,7 +298,7 @@ static unsigned int
>  	last_irq_sums [NR_CPUS],
>  	alert_counter [NR_CPUS];
>  
> -void touch_nmi_watchdog (void)
> +void touch_nmi_watchdog(void)
>  {
>  	if (nmi_watchdog > 0) {
>  		unsigned cpu;
> @@ -307,8 +307,10 @@ void touch_nmi_watchdog (void)
>  		 * Just reset the alert counters, (other CPUs might be
>  		 * spinning on locks we hold):
>  		 */
> -		for_each_present_cpu (cpu)
> -			alert_counter[cpu] = 0;
> +		for_each_present_cpu(cpu) {
> +			if (alert_counter[cpu])
> +				alert_counter[cpu] = 0;
> +		}
>  	}
>  
>  	/*
> 
> So I'd be inclined to simplify your patch to a bare
> 
> From: Konrad Rzeszutek <konrad@darnok.org>
> 
> On large memory configuration with not so fast CPUs the NMI watchdog is
> triggered when memory addresses are being gathered and printed.  The code
> paths for Alt-SysRq-t are sprinkled with touch_nmi_watchdog in various
> places but not in this routine (or in the loop that utilizes this
> function).  The patch has been tested for regression on large CPU+memory
> configuration (128 logical CPUs + 224 GB) and 1,2,4,16-CPU sockets with
> various memory sizes (1,2,4,6,20).
> 
> Cc: Andi Kleen <ak@suse.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  arch/x86_64/kernel/traps.c |    1 +
>  1 files changed, 1 insertion(+)
> 
> diff -puN arch/x86_64/kernel/traps.c~inhibit-nmi-watchdog-when-alt-sysrq-t-operation-is-underway arch/x86_64/kernel/traps.c
> --- a/arch/x86_64/kernel/traps.c~inhibit-nmi-watchdog-when-alt-sysrq-t-operation-is-underway
> +++ a/arch/x86_64/kernel/traps.c
> @@ -397,6 +397,7 @@ static int print_trace_stack(void *data,
>  
>  static void print_trace_address(void *data, unsigned long addr)
>  {
> +	touch_nmi_watchdog();
>  	printk_address(addr);
>  }
>  
> _
> 
> 
> OK?

Re: [PATCH] Inhibit NMI watchdog when Alt-SysRq-T operation is underway.

[Andrew Morton]

On Fri, 20 Jul 2007 07:18:23 -0400
Konrad Rzeszutek <konrad@darnok.org> wrote:

> I tested your patch along with mine and found two things out:
> 
>  1). Missing this patch (for i386 platform)
> 
> diff --git a/arch/i386/kernel/traps.c b/arch/i386/kernel/traps.c
> index 90da057..9f3a7ff 100644
> --- a/arch/i386/kernel/traps.c
> +++ b/arch/i386/kernel/traps.c
> @@ -207,6 +207,7 @@ static void print_trace_address(void *da
>  {
>  	printk("%s [<%08lx>] ", (char *)data, addr);
>  	print_symbol("%s\n", addr);
> +	touch_nmi_watchdog();
>  }

ok...

>  static struct stacktrace_ops print_trace_ops = {
> 
> 
>   2). If I run Alt-SysRq-t about 5000 times in a loop, the slow down 
>       with this change is about 5%. Is this a big issue? (This was
>       testing both i686 and x86_64).
> 

I'm surprised.  I assume this was when it was printing to a high-speed
device?  console or netconsole?

I don't think we need to spend too much time optimising sysrq-T performance ;)