[PATCH] adjust x86-64 watchdog tick calculation

[PATCH] adjust x86-64 watchdog tick calculation

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

(Note: Patch also attached because the inline version is certain to get
line wrapped.)

Get the x86-64 watchdog tick calculation into a state where it can also
be used with nmi_hz other than 1Hz. Also do not turn on the watchdog by
default (as is already done on i386).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

diff -Npru linux-2.6.12-rc4.base/arch/x86_64/kernel/nmi.c linux-2.6.12-rc4/arch/x86_64/kernel/nmi.c
--- linux-2.6.12-rc4.base/arch/x86_64/kernel/nmi.c	2005-05-11 17:27:54.848855552 +0200
+++ linux-2.6.12-rc4/arch/x86_64/kernel/nmi.c	2005-05-11 17:50:36.257889920 +0200
@@ -57,7 +57,7 @@ static unsigned int lapic_nmi_owner;
 int nmi_active;		/* oprofile uses this */
 int panic_on_timeout;
 
-unsigned int nmi_watchdog = NMI_DEFAULT;
+unsigned int nmi_watchdog = NMI_NONE;
 static unsigned int nmi_hz = HZ;
 unsigned int nmi_perfctr_msr;	/* the MSR to reset in NMI handler */
 
@@ -325,7 +325,7 @@ static void setup_k7_watchdog(void)
 		| K7_NMI_EVENT;
 
 	wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
-	wrmsrl(MSR_K7_PERFCTR0, -((u64)cpu_khz*1000) / nmi_hz);
+	wrmsrl(MSR_K7_PERFCTR0, -((u64)cpu_khz * 1000 / nmi_hz));
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 	evntsel |= K7_EVNTSEL_ENABLE;
 	wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
@@ -404,7 +404,7 @@ void nmi_watchdog_tick (struct pt_regs *
 		alert_counter[cpu] = 0;
 	}
 	if (nmi_perfctr_msr)
-		wrmsr(nmi_perfctr_msr, -(cpu_khz/nmi_hz*1000), -1);
+		wrmsrl(nmi_perfctr_msr, -((u64)cpu_khz * 1000 / nmi_hz));
 }
 
 static int dummy_nmi_callback(struct pt_regs * regs, int cpu)



[-- Attachment #2: linux-2.6.12-rc4-x86_64-watchdog.patch --]
[-- Type: text/plain, Size: 1540 bytes --]

(Note: Patch also attached because the inline version is certain to get
line wrapped.)

Get the x86-64 watchdog tick calculation into a state where it can also
be used with nmi_hz other than 1Hz. Also do not turn on the watchdog by
default (as is already done on i386).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

diff -Npru linux-2.6.12-rc4.base/arch/x86_64/kernel/nmi.c linux-2.6.12-rc4/arch/x86_64/kernel/nmi.c
--- linux-2.6.12-rc4.base/arch/x86_64/kernel/nmi.c	2005-05-11 17:27:54.848855552 +0200
+++ linux-2.6.12-rc4/arch/x86_64/kernel/nmi.c	2005-05-11 17:50:36.257889920 +0200
@@ -57,7 +57,7 @@ static unsigned int lapic_nmi_owner;
 int nmi_active;		/* oprofile uses this */
 int panic_on_timeout;
 
-unsigned int nmi_watchdog = NMI_DEFAULT;
+unsigned int nmi_watchdog = NMI_NONE;
 static unsigned int nmi_hz = HZ;
 unsigned int nmi_perfctr_msr;	/* the MSR to reset in NMI handler */
 
@@ -325,7 +325,7 @@ static void setup_k7_watchdog(void)
 		| K7_NMI_EVENT;
 
 	wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
-	wrmsrl(MSR_K7_PERFCTR0, -((u64)cpu_khz*1000) / nmi_hz);
+	wrmsrl(MSR_K7_PERFCTR0, -((u64)cpu_khz * 1000 / nmi_hz));
 	apic_write(APIC_LVTPC, APIC_DM_NMI);
 	evntsel |= K7_EVNTSEL_ENABLE;
 	wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
@@ -404,7 +404,7 @@ void nmi_watchdog_tick (struct pt_regs *
 		alert_counter[cpu] = 0;
 	}
 	if (nmi_perfctr_msr)
-		wrmsr(nmi_perfctr_msr, -(cpu_khz/nmi_hz*1000), -1);
+		wrmsrl(nmi_perfctr_msr, -((u64)cpu_khz * 1000 / nmi_hz));
 }
 
 static int dummy_nmi_callback(struct pt_regs * regs, int cpu)

Re: [PATCH] adjust x86-64 watchdog tick calculation

[Alexander Nyberg]

tor 2005-05-12 klockan 10:27 +0200 skrev Jan Beulich:
> (Note: Patch also attached because the inline version is certain to get
> line wrapped.)
> 
> Get the x86-64 watchdog tick calculation into a state where it can also
> be used with nmi_hz other than 1Hz. Also do not turn on the watchdog by
> default (as is already done on i386).
> 

Why shouldn't the watchdog be turned on by default? It's an extremely
useful debugging aid and it's not like it fires NMIs often (the nmi_hz
is far from reality).

> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> diff -Npru linux-2.6.12-rc4.base/arch/x86_64/kernel/nmi.c linux-2.6.12-rc4/arch/x86_64/kernel/nmi.c
> --- linux-2.6.12-rc4.base/arch/x86_64/kernel/nmi.c	2005-05-11 17:27:54.848855552 +0200
> +++ linux-2.6.12-rc4/arch/x86_64/kernel/nmi.c	2005-05-11 17:50:36.257889920 +0200
> @@ -57,7 +57,7 @@ static unsigned int lapic_nmi_owner;
>  int nmi_active;		/* oprofile uses this */
>  int panic_on_timeout;
>  
> -unsigned int nmi_watchdog = NMI_DEFAULT;
> +unsigned int nmi_watchdog = NMI_NONE;
>  static unsigned int nmi_hz = HZ;
>  unsigned int nmi_perfctr_msr;	/* the MSR to reset in NMI handler */
>  
> @@ -325,7 +325,7 @@ static void setup_k7_watchdog(void)
>  		| K7_NMI_EVENT;
>  
>  	wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
> -	wrmsrl(MSR_K7_PERFCTR0, -((u64)cpu_khz*1000) / nmi_hz);
> +	wrmsrl(MSR_K7_PERFCTR0, -((u64)cpu_khz * 1000 / nmi_hz));
>  	apic_write(APIC_LVTPC, APIC_DM_NMI);
>  	evntsel |= K7_EVNTSEL_ENABLE;
>  	wrmsr(MSR_K7_EVNTSEL0, evntsel, 0);
> @@ -404,7 +404,7 @@ void nmi_watchdog_tick (struct pt_regs *
>  		alert_counter[cpu] = 0;
>  	}
>  	if (nmi_perfctr_msr)
> -		wrmsr(nmi_perfctr_msr, -(cpu_khz/nmi_hz*1000), -1);
> +		wrmsrl(nmi_perfctr_msr, -((u64)cpu_khz * 1000 / nmi_hz));
>  }
>  
>  static int dummy_nmi_callback(struct pt_regs * regs, int cpu)
> 
> 

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Andi Kleen]

On Thu, May 12, 2005 at 12:00:08PM +0200, Alexander Nyberg wrote:
> tor 2005-05-12 klockan 10:27 +0200 skrev Jan Beulich:
> > (Note: Patch also attached because the inline version is certain to get
> > line wrapped.)
> > 
> > Get the x86-64 watchdog tick calculation into a state where it can also
> > be used with nmi_hz other than 1Hz. Also do not turn on the watchdog by
> > default (as is already done on i386).
> > 
> 
> Why shouldn't the watchdog be turned on by default? It's an extremely
> useful debugging aid and it's not like it fires NMIs often (the nmi_hz
> is far from reality).

I agree, I definitely want to keep the watchdog enabled by default.
It is a invaluable debugging aid.

The only reason i386 has it turned off by default is that it did not
always work on some oudated hardware. Needs to be probably revisited
at some point too.

-Andi

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Pavel Machek]

Hi!

> > (Note: Patch also attached because the inline version is certain to get
> > line wrapped.)
> > 
> > Get the x86-64 watchdog tick calculation into a state where it can also
> > be used with nmi_hz other than 1Hz. Also do not turn on the watchdog by
> > default (as is already done on i386).
> > 
> 
> Why shouldn't the watchdog be turned on by default? It's an extremely
> useful debugging aid and it's not like it fires NMIs often (the nmi_hz
> is far from reality).

Because it kills machine when interrupt latency gets too high?
Like reading battery status using i2c...
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Andi Kleen]

> Because it kills machine when interrupt latency gets too high?
> Like reading battery status using i2c...

That's a bug in the I2C reader then. Don't shot the messenger for bad news.


-Andi

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Pavel Machek]

On Pá 13-05-05 13:30:23, Andi Kleen wrote:
> > Because it kills machine when interrupt latency gets too high?
> > Like reading battery status using i2c...
> 
> That's a bug in the I2C reader then. Don't shot the messenger for bad news.

Disagreed.

Linux is not real time OS. Perhaps some real-time constraints "may not
spend > 100msec with interrupts disabled" would be healthy, but it
certainly needs more discussion than "lets enable NMI
watchdog.". It needs to be written somewhere in big bold letters, too.

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Lee Revell]

On Fri, 2005-05-13 at 21:52 +0200, Pavel Machek wrote:
> On Pá 13-05-05 13:30:23, Andi Kleen wrote:
> > > Because it kills machine when interrupt latency gets too high?
> > > Like reading battery status using i2c...
> > 
> > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> 
> Disagreed.
> 
> Linux is not real time OS. Perhaps some real-time constraints "may not
> spend > 100msec with interrupts disabled" would be healthy
             ^^^^
You mean "microseconds", right?  100ms will be perceived by the user as,
well, their machine freezing for 100ms...

Lee

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Pavel Machek]

Hi!

> > > > Because it kills machine when interrupt latency gets too high?
> > > > Like reading battery status using i2c...
> > > 
> > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> > 
> > Disagreed.
> > 
> > Linux is not real time OS. Perhaps some real-time constraints "may not
> > spend > 100msec with interrupts disabled" would be healthy
>              ^^^^
> You mean "microseconds", right?  100ms will be perceived by the user as,
> well, their machine freezing for 100ms...

I did mean miliseconds. IIRC current watchdog is at one second and it
still triggers even in cases when operation just takes too long.
								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Lee Revell]

On Sat, 2005-05-14 at 00:51 +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > Because it kills machine when interrupt latency gets too high?
> > > > > Like reading battery status using i2c...
> > > > 
> > > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> > > 
> > > Disagreed.
> > > 
> > > Linux is not real time OS. Perhaps some real-time constraints "may not
> > > spend > 100msec with interrupts disabled" would be healthy
> >              ^^^^
> > You mean "microseconds", right?  100ms will be perceived by the user as,
> > well, their machine freezing for 100ms...
> 
> I did mean miliseconds. IIRC current watchdog is at one second and it
> still triggers even in cases when operation just takes too long.

I thought there was an understanding that 1 ms would be the target for
desktop responsiveness.  So yes, disabling interrupts for more than 1ms
is considered a bug.

Why do you need to disable interrupts for 100ms to read the battery
status exactly?

Lee

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Pavel Machek]

Hi!

> > > > > > Because it kills machine when interrupt latency gets too high?
> > > > > > Like reading battery status using i2c...
> > > > > 
> > > > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> > > > 
> > > > Disagreed.
> > > > 
> > > > Linux is not real time OS. Perhaps some real-time constraints "may not
> > > > spend > 100msec with interrupts disabled" would be healthy
> > >              ^^^^
> > > You mean "microseconds", right?  100ms will be perceived by the user as,
> > > well, their machine freezing for 100ms...
> > 
> > I did mean miliseconds. IIRC current watchdog is at one second and it
> > still triggers even in cases when operation just takes too long.
> 
> I thought there was an understanding that 1 ms would be the target for
> desktop responsiveness.  So yes, disabling interrupts for more than 1ms
> is considered a bug.

I do not think so.

In may be "worth fixing", but no, that does not mean you should stick
"if ints_disabled > 1msec panic()" into code. That would make most
systems unusable.

Think pio-only disks, for example. Think serial console.
									Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Dave Jones]

On Fri, May 13, 2005 at 06:56:33PM -0400, Lee Revell wrote:
 > On Sat, 2005-05-14 at 00:51 +0200, Pavel Machek wrote:
 > > Hi!
 > > 
 > > > > > > Because it kills machine when interrupt latency gets too high?
 > > > > > > Like reading battery status using i2c...
 > > > > > 
 > > > > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
 > > > > 
 > > > > Disagreed.
 > > > > 
 > > > > Linux is not real time OS. Perhaps some real-time constraints "may not
 > > > > spend > 100msec with interrupts disabled" would be healthy
 > > >              ^^^^
 > > > You mean "microseconds", right?  100ms will be perceived by the user as,
 > > > well, their machine freezing for 100ms...
 > > 
 > > I did mean miliseconds. IIRC current watchdog is at one second and it
 > > still triggers even in cases when operation just takes too long.
 > 
 > I thought there was an understanding that 1 ms would be the target for
 > desktop responsiveness.  So yes, disabling interrupts for more than 1ms
 > is considered a bug.
 > 
 > Why do you need to disable interrupts for 100ms to read the battery
 > status exactly?

On some unfortunate hardware, we can go away even longer whilst
the BIOS does various SMI voodoo.  It got so bad in some situations
that the maintainers of the gnome battery app lowered the frequency
at which the poll the acpi interface.

		Dave

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Andi Kleen]

On Fri, May 13, 2005 at 06:56:33PM -0400, Lee Revell wrote:
> On Sat, 2005-05-14 at 00:51 +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > Because it kills machine when interrupt latency gets too high?
> > > > > > Like reading battery status using i2c...
> > > > > 
> > > > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> > > > 
> > > > Disagreed.
> > > > 
> > > > Linux is not real time OS. Perhaps some real-time constraints "may not
> > > > spend > 100msec with interrupts disabled" would be healthy
> > >              ^^^^
> > > You mean "microseconds", right?  100ms will be perceived by the user as,
> > > well, their machine freezing for 100ms...
> > 
> > I did mean miliseconds. IIRC current watchdog is at one second and it
> > still triggers even in cases when operation just takes too long.
> 
> I thought there was an understanding that 1 ms would be the target for
> desktop responsiveness.  So yes, disabling interrupts for more than 1ms
> is considered a bug.

No, it's a bit different. Let's say disabling interrupts after
boot even for considerable fractions of 1ms is a bug. But then
there are exceptional circumstances where you have no other choice.
In that case you need to use touch_nmi_watchdog yourself.
But these things should be rare, e.g. only in unlikely error
handling situations.

> 
> Why do you need to disable interrupts for 100ms to read the battery
> status exactly?

I guess because he's too lazy to rewrite the code use semaphores
and schedule_timeout(). He just needs to get over that. 

-Andi

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Andi Kleen]

On Fri, May 13, 2005 at 09:52:15PM +0200, Pavel Machek wrote:
> On P? 13-05-05 13:30:23, Andi Kleen wrote:
> > > Because it kills machine when interrupt latency gets too high?
> > > Like reading battery status using i2c...
> > 
> > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> 
> Disagreed.
> 
> Linux is not real time OS. Perhaps some real-time constraints "may not
> spend > 100msec with interrupts disabled" would be healthy, but it
> certainly needs more discussion than "lets enable NMI
> watchdog.". It needs to be written somewhere in big bold letters, too.

While linux is not a real time OS it has been always known that
turning off interrupts for a long time is extremly rude.

If you really want you can use touch_nmi_watchdog in the delay
loop then.  But note you have to compile it in, because touch_nmi_watchdog
is not exported (Linus vetoed that for good reasons).

But again do you really need to disable interrupts during this
i2c access? Can't you just use a schedule_timeout() and a semaphore?
Why would other interrupts cause a problem during such a long delay?

-Andi

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Pavel Machek]

Hi!

> > > > Because it kills machine when interrupt latency gets too high?
> > > > Like reading battery status using i2c...
> > > 
> > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> > 
> > Disagreed.
> > 
> > Linux is not real time OS. Perhaps some real-time constraints "may not
> > spend > 100msec with interrupts disabled" would be healthy, but it
> > certainly needs more discussion than "lets enable NMI
> > watchdog.". It needs to be written somewhere in big bold letters, too.
> 
> While linux is not a real time OS it has been always known that
> turning off interrupts for a long time is extremly rude.

Yes, it is rude, but it should not panic machines.

> If you really want you can use touch_nmi_watchdog in the delay
> loop then.  But note you have to compile it in, because touch_nmi_watchdog
> is not exported (Linus vetoed that for good reasons).
> 
> But again do you really need to disable interrupts during this
> i2c access? Can't you just use a schedule_timeout() and a semaphore?
> Why would other interrupts cause a problem during such a long delay?

In this case it is "AML code told you to disable interrupts, and do
this kind of bitbang". AML interpretter has no idea of what that code
does... Perhaps we could sprinkle touch_nmi_watchdog all over the
interpretter, but that's just ugly.
								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [discuss] Re: [PATCH] adjust x86-64 watchdog tick calculation

[Andi Kleen]

On Sun, May 15, 2005 at 12:51:00PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > Because it kills machine when interrupt latency gets too high?
> > > > > Like reading battery status using i2c...
> > > > 
> > > > That's a bug in the I2C reader then. Don't shot the messenger for bad news.
> > > 
> > > Disagreed.
> > > 
> > > Linux is not real time OS. Perhaps some real-time constraints "may not
> > > spend > 100msec with interrupts disabled" would be healthy, but it
> > > certainly needs more discussion than "lets enable NMI
> > > watchdog.". It needs to be written somewhere in big bold letters, too.
> > 
> > While linux is not a real time OS it has been always known that
> > turning off interrupts for a long time is extremly rude.
> 
> Yes, it is rude, but it should not panic machines.

Disagreed. It is a bug and needs to panic machines.

> 
> > If you really want you can use touch_nmi_watchdog in the delay
> > loop then.  But note you have to compile it in, because touch_nmi_watchdog
> > is not exported (Linus vetoed that for good reasons).
> > 
> > But again do you really need to disable interrupts during this
> > i2c access? Can't you just use a schedule_timeout() and a semaphore?
> > Why would other interrupts cause a problem during such a long delay?
> 
> In this case it is "AML code told you to disable interrupts, and do
> this kind of bitbang". AML interpretter has no idea of what that code
> does... Perhaps we could sprinkle touch_nmi_watchdog all over the
> interpretter, but that's just ugly.

Actually that's wrong. Because I fixed that code long ago if
you mean the access in battery.c
(if you search the ACPI mailing list for my name you should find it)
And there was no AML code involved here.

For some reason the ACPI guys didn't merge my patch though,
but that just needs to be revisited.

-Andi