[2.6.4-rc2] bogus semicolon behind if()

[2.6.4-rc2] bogus semicolon behind if()

[Thomas Schlichter]

Hi,

recently I found a bogus semicolon an if statement. So I was searching for 
other possible problems with following command:

find linux-2.6.4-rc2 -name "*.c" -exec grep -Hn "\<if[[:space:]]*(\([^()]\|
([^()]*)\)*)[[:space:]]*;" {} \;

That found following possible problems:

linux-2.6.4-rc2/arch/um/kernel/tt/tracer.c:257:		if(WIFEXITED(status)) ;
linux-2.6.4-rc2/arch/i386/kernel/io_apic.c:2200:				if (check_nmi_watchdog() < 
0);
linux-2.6.4-rc2/arch/i386/kernel/io_apic.c:2224:				if (check_nmi_watchdog() < 
0);
linux-2.6.4-rc2/drivers/atm/iphase.c:155:     if (!desc1) ;
linux-2.6.4-rc2/drivers/net/wan/pc300_drv.c:3664:			if (card->chan[i].d.dev);
linux-2.6.4-rc2/drivers/net/tokenring/ibmtr.c:613:	else if 
(ti->shared_ram_paging == 0xf);  /* No paging in adapter */
linux-2.6.4-rc2/drivers/usb/misc/speedtch.c:92:#define DEBUG_ON(x)	do { if 
(x); } while (0)
linux-2.6.4-rc2/drivers/usb/media/w9968cf.c:3374:		if (tuner.tuner != 0);
linux-2.6.4-rc2/drivers/isdn/capi/capiutil.c:438:	else if (c <= 0x0f);
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_sx.c:385:	if (Read_hfc(cs, 
HFCSX_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_sx.c:415:	if (Read_hfc(cs, 
HFCSX_INT_S2));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_sx.c:1290:						if (Read_hfc(cs, 
HFCSX_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_pci.c:131:	if (Read_hfc(cs, 
HFCPCI_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_pci.c:161:	if (Read_hfc(cs, 
HFCPCI_INT_S1));
linux-2.6.4-rc2/drivers/isdn/hisax/hfc_pci.c:1543:						if (Read_hfc(cs, 
HFCPCI_INT_S1));
linux-2.6.4-rc2/drivers/s390/scsi/zfcp_erp.c:2057:		if (status == 
ZFCP_ERP_SUCCEEDED) ;	/* no further action */
linux-2.6.4-rc2/drivers/scsi/sg.c:356:	if (ppos != &filp->f_pos) ;	/* FIXME: 
Hmm.  Seek to the right place, or fail?  */
linux-2.6.4-rc2/drivers/scsi/sg.c:514:	if (ppos != &filp->f_pos) ;	/* FIXME: 
Hmm.  Seek to the right place, or fail?  */

Best regards
   Thomas Schlichter

P.S.: Wouldn't it be nice if gcc complained about these mistakes?

Re: [2.6.4-rc2] bogus semicolon behind if()

[Andreas Schwab]

Thomas Schlichter <thomas.schlichter@web.de> writes:

> P.S.: Wouldn't it be nice if gcc complained about these mistakes?

Among these 18 cases are 13 false positives. :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [2.6.4-rc2] bogus semicolon behind if()

[Thomas Schlichter]

Am Dienstag, 9. März 2004 00:43 schrieb Andreas Schwab:
> Thomas Schlichter <thomas.schlichter@web.de> writes:
> > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
>
> Among these 18 cases are 13 false positives. :-)

Yes, but at least it's a very bad coding style, and a warning may be useful to 
find the _real_ bugs...

  Thomas

Re: [2.6.4-rc2] bogus semicolon behind if()

[Andrew Morton]

Andreas Schwab <schwab@suse.de> wrote:
>
> Thomas Schlichter <thomas.schlichter@web.de> writes:
> 
> > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
> 
> Among these 18 cases are 13 false positives. :-)

Rename Thomas's script to crappy-code-detector.sh and its hit rate goes to
100% ;)

Re: [2.6.4-rc2] bogus semicolon behind if()

[Philippe Elie]

On Mon, 08 Mar 2004 at 16:29 +0000, Andrew Morton wrote:

> Andreas Schwab <schwab@suse.de> wrote:
> >
> > Thomas Schlichter <thomas.schlichter@web.de> writes:
> > 
> > > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
> > 
> > Among these 18 cases are 13 false positives. :-)
> 
> Rename Thomas's script to crappy-code-detector.sh and its hit rate goes to
> 100% ;)
> 

Was this patch so crappy ? http://tinyurl.com/2jbe4 , 

-				check_nmi_watchdog();
+				if (check_nmi_watchdog() < 0);
+					timer_ack = !cpu_has_tsc


regards,
Phil

Re: [2.6.4-rc2] bogus semicolon behind if()

[Thomas Schlichter]

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

Am Dienstag, 9. März 2004 08:01 schrieb Philippe Elie:
> On Mon, 08 Mar 2004 at 16:29 +0000, Andrew Morton wrote:

~~ snip ~~

> > Rename Thomas's script to crappy-code-detector.sh and its hit rate goes
> > to 100% ;)
>
> Was this patch so crappy ? http://tinyurl.com/2jbe4 ,
>
> -				check_nmi_watchdog();
> +				if (check_nmi_watchdog() < 0);
> +					timer_ack = !cpu_has_tsc

Well, exactly this code made me look for other possible problems... ;-)
As I wrote a few days ago I have problems with that ChangeSet,
  (http://marc.theaimsgroup.com/?l=linux-kernel&m=107840458123059&w=2)
so I did examine it closer.

My results are:
1. The semicolons behind the if()'s cannot be there intentionally.
2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and 
as my CPU has a TSC, this cannot be correct for me:
-	timer_ack = 1;
+	if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
+		timer_ack = 1;
+	else
+		timer_ack = !cpu_has_tsc;

I changed that if(...) to
	if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
which works fine for me here, but I am not 100% sure if this is what the 
author of the original patch ment and if it still fixes the original 
problem...

The patch which makes these changes is attached. Perhaps someone else wants to 
test it, too...

   Thomas Schlichter

P.S.: I tested it with an AMD Athlon 3000+ on a board with a VIA KT400 chipset 
and enabled ACPI and IO-APIC.

[-- Attachment #2: fix-8259-timer-ack.diff --]
[-- Type: text/x-diff, Size: 1021 bytes --]

--- linux-2.6.4-rc2-mm1/arch/i386/kernel/io_apic.c.orig	2004-03-08 16:29:14.000000000 +0100
+++ linux-2.6.4-rc2-mm1/arch/i386/kernel/io_apic.c	2004-03-08 17:26:40.000000000 +0100
@@ -2181,7 +2181,7 @@ static inline void check_timer(void)
 	 */
 	apic_write_around(APIC_LVT0, APIC_LVT_MASKED | APIC_DM_EXTINT);
 	init_8259A(1);
-	if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
+	if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
 		timer_ack = 1;
 	else
 		timer_ack = !cpu_has_tsc;
@@ -2202,7 +2202,7 @@ static inline void check_timer(void)
 				disable_8259A_irq(0);
 				setup_nmi();
 				enable_8259A_irq(0);
-				if (check_nmi_watchdog() < 0);
+				if (check_nmi_watchdog() < 0)
 					timer_ack = !cpu_has_tsc;
 			}
 			return;
@@ -2226,7 +2226,7 @@ static inline void check_timer(void)
 				add_pin_to_irq(0, 0, pin2);
 			if (nmi_watchdog == NMI_IO_APIC) {
 				setup_nmi();
-				if (check_nmi_watchdog() < 0);
+				if (check_nmi_watchdog() < 0)
 					timer_ack = !cpu_has_tsc;
 			}
 			return;

Re: [2.6.4-rc2] bogus semicolon behind if()

[Philippe Elie]

On Tue, 09 Mar 2004 at 12:08 +0000, Thomas Schlichter wrote:

> As I wrote a few days ago I have problems with that ChangeSet,
>   (http://marc.theaimsgroup.com/?l=linux-kernel&m=107840458123059&w=2)
> so I did examine it closer.

errmm, http://tinyurl.com/2jbe4

Maciej, you wrote this patch, any comment ?

> so I did examine it closer.
> 
> My results are:
> 1. The semicolons behind the if()'s cannot be there intentionally.
> 2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and 
> as my CPU has a TSC, this cannot be correct for me:
> -	timer_ack = 1;
> +	if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
> +		timer_ack = 1;
> +	else
> +		timer_ack = !cpu_has_tsc;

I don't get the figure, this code in check_timer() is called by
setup_IO_APIC so APIC_INTEGRATED(ver) is always 0 ?


> I changed that if(...) to
> 	if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
> which works fine for me here, but I am not 100% sure if this is what the 
> author of the original patch ment and if it still fixes the original 
> problem...
 

regards,
Phil

Re: [2.6.4-rc2] bogus semicolon behind if()

[Zwane Mwaikambo]

On Wed, 10 Mar 2004, Philippe Elie wrote:

> > 1. The semicolons behind the if()'s cannot be there intentionally.
> > 2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and
> > as my CPU has a TSC, this cannot be correct for me:
> > -	timer_ack = 1;
> > +	if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
> > +		timer_ack = 1;
> > +	else
> > +		timer_ack = !cpu_has_tsc;
>
> I don't get the figure, this code in check_timer() is called by
> setup_IO_APIC so APIC_INTEGRATED(ver) is always 0 ?

Nope, that will catch the older external "local" APICs like the 82489s (P5
etc), most IOAPICs will return 0x11 or higher for version#

Re: [2.6.4-rc2] bogus semicolon behind if()

[Maciej W. Rozycki]

On Wed, 10 Mar 2004, Philippe Elie wrote:

> > As I wrote a few days ago I have problems with that ChangeSet,
> >   (http://marc.theaimsgroup.com/?l=linux-kernel&m=107840458123059&w=2)
> > so I did examine it closer.
> 
> errmm, http://tinyurl.com/2jbe4
> 
> Maciej, you wrote this patch, any comment ?

 Yep, that's a stupid typo, but the bug would only trigger for a system
that would have:

1. a discrete 82489DX APIC,

2. a functional TSC,

3. a timer interrupt working through the I/O APIC,

4. a working I/O APIC NMI watchdog.

Such systems used to actually exist, but you'd have a hard time trying to
find one.

 Here's an obvious update.  Thomas, thanks for spotting it.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-2.6.4-timer_ack-fix-0
diff -up --recursive --new-file linux-2.6.4.macro/arch/i386/kernel/io_apic.c linux-2.6.4/arch/i386/kernel/io_apic.c
--- linux-2.6.4.macro/arch/i386/kernel/io_apic.c	2004-03-17 17:09:29.000000000 +0000
+++ linux-2.6.4/arch/i386/kernel/io_apic.c	2004-03-17 17:11:07.000000000 +0000
@@ -2195,7 +2195,7 @@ static inline void check_timer(void)
 				disable_8259A_irq(0);
 				setup_nmi();
 				enable_8259A_irq(0);
-				if (check_nmi_watchdog() < 0);
+				if (check_nmi_watchdog() < 0)
 					timer_ack = !cpu_has_tsc;
 			}
 			return;
@@ -2219,7 +2219,7 @@ static inline void check_timer(void)
 				add_pin_to_irq(0, 0, pin2);
 			if (nmi_watchdog == NMI_IO_APIC) {
 				setup_nmi();
-				if (check_nmi_watchdog() < 0);
+				if (check_nmi_watchdog() < 0)
 					timer_ack = !cpu_has_tsc;
 			}
 			return;

Re: [2.6.4-rc2] bogus semicolon behind if()

[Maciej W. Rozycki]

On Tue, 9 Mar 2004, Thomas Schlichter wrote:

> 2. To fix my problem, timer_ack must be set to 1 for my (integrated) APIC, and 
> as my CPU has a TSC, this cannot be correct for me:
> -	timer_ack = 1;
> +	if (nmi_watchdog == NMI_IO_APIC && !APIC_INTEGRATED(ver))
> +		timer_ack = 1;
> +	else
> +		timer_ack = !cpu_has_tsc;
> 
> I changed that if(...) to
> 	if (nmi_watchdog == NMI_IO_APIC || APIC_INTEGRATED(ver))
> which works fine for me here, but I am not 100% sure if this is what the 
> author of the original patch ment and if it still fixes the original 
> problem...

 You need timer_ack set to one when either:

1. you use the I/O APIC NMI watchdog and you have a discrete APIC chip
(i.e. the 82489DX),

or:

2. the timer interrupt (IRQ 0) goes through one of the APICs (whatever
way; we check three variations) and the TSC is non-functional (absent or 
disabled).

Since you have an integrated APIC and you use the TSC, you may have 
timer_ack set to zero.  That saves a few (possibly slow) I/O accesses and 
works around problems that may arise due 8259A clone (in)compatibility or 
bugs in SMM firmware.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [2.6.4-rc2] bogus semicolon behind if()

[Andrew Morton]

"Maciej W. Rozycki" <macro@ds2.pg.gda.pl> wrote:
>
>  You need timer_ack set to one when either:
> 
> 1. you use the I/O APIC NMI watchdog and you have a discrete APIC chip
> (i.e. the 82489DX),
> 
> or:
> 
> 2. the timer interrupt (IRQ 0) goes through one of the APICs (whatever
> way; we check three variations) and the TSC is non-functional (absent or 
> disabled).
> 

I still have a couple of NMI patches in -mm:

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch

What should we do with these?

Re: [2.6.4-rc2] bogus semicolon behind if()

[Maciej W. Rozycki]

On Wed, 17 Mar 2004, Andrew Morton wrote:

> I still have a couple of NMI patches in -mm:
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
> 
> What should we do with these?

 I think we should ask Mikael Pettersson as he is the local APIC watchdog 
expert.  Mikael?

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [2.6.4-rc2] bogus semicolon behind if()

[Mikael Pettersson]

Maciej W. Rozycki writes:
 > On Wed, 17 Mar 2004, Andrew Morton wrote:
 > 
 > > I still have a couple of NMI patches in -mm:
 > > 
 > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
 > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
 > > 
 > > What should we do with these?
 > 
 >  I think we should ask Mikael Pettersson as he is the local APIC watchdog 
 > expert.  Mikael?

Will do. Is there a problem with them, or do you just want them
reviewed for merging into 2.6.5-rc?

/Mikael

Re: [2.6.4-rc2] bogus semicolon behind if()

[Andrew Morton]

Mikael Pettersson <mikpe@csd.uu.se> wrote:
>
> Maciej W. Rozycki writes:
>  > On Wed, 17 Mar 2004, Andrew Morton wrote:
>  > 
>  > > I still have a couple of NMI patches in -mm:
>  > > 
>  > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
>  > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
>  > > 
>  > > What should we do with these?
>  > 
>  >  I think we should ask Mikael Pettersson as he is the local APIC watchdog 
>  > expert.  Mikael?
> 
> Will do. Is there a problem with them, or do you just want them
> reviewed for merging into 2.6.5-rc?
> 

They seem to work OK - I did a batch of testing with various setups.  But a
retest wouldn't hurt.

We mainly need a review and general finish-it-off-and-bless-it please.

Re: [2.6.4-rc2] bogus semicolon behind if()

[Zwane Mwaikambo]

On Wed, 17 Mar 2004, Andrew Morton wrote:

> I still have a couple of NMI patches in -mm:
>
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch

That looks like a decent bug fix.

> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch

Didn't _you_ want that one? ;)

Re: [2.6.4-rc2] bogus semicolon behind if()

[Zwane Mwaikambo]

On Sat, 20 Mar 2004, Zwane Mwaikambo wrote:

> On Wed, 17 Mar 2004, Andrew Morton wrote:
>
> > I still have a couple of NMI patches in -mm:
> >
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
>
> That looks like a decent bug fix.
>
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
>
> Didn't _you_ want that one? ;)

Excuse this little outburt, i seem to be reading an old mbox.

Re: [2.6.4-rc2] bogus semicolon behind if()

[Thomas Schlichter]

Am Mittwoch, 17. März 2004 17:51 schrieb Maciej W. Rozycki:

~~ snip ~~

>  You need timer_ack set to one when either:
>
> 1. you use the I/O APIC NMI watchdog and you have a discrete APIC chip
> (i.e. the 82489DX),
>
> or:
>
> 2. the timer interrupt (IRQ 0) goes through one of the APICs (whatever
> way; we check three variations) and the TSC is non-functional (absent or
> disabled).
>
> Since you have an integrated APIC and you use the TSC, you may have
> timer_ack set to zero.  That saves a few (possibly slow) I/O accesses and
> works around problems that may arise due 8259A clone (in)compatibility or
> bugs in SMM firmware.
>
>   Maciej

Well, my timer interrupt goes through the IO-APIC but I do have a functional 
TSC. Nevertheless my system requires timer_ack to be set... If it isn't, my 
CPU does not utilize its C2 state...

  Thomas

Re: [2.6.4-rc2] bogus semicolon behind if()

[Maciej W. Rozycki]

On Fri, 19 Mar 2004, Thomas Schlichter wrote:

> Well, my timer interrupt goes through the IO-APIC but I do have a functional 
> TSC. Nevertheless my system requires timer_ack to be set... If it isn't, my 
> CPU does not utilize its C2 state...

 Hmm, I wonder if there's any relationship between the state of the local
APIC and your observation.  Can you please see if the following hack
changes anything (this assumes you have your timer IRQ directly connected
to an I/O APIC input)?

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

--- io_apic.c.macro	2004-03-19 20:13:44.000000000 +0000
+++ io_apic.c	2004-03-19 20:15:23.000000000 +0000
@@ -1613,7 +1613,7 @@ static inline void check_timer(void)
 		timer_ack = 1;
 	else
 		timer_ack = !cpu_has_tsc;
-	enable_8259A_irq(0);
+	disable_8259A_irq(0);
 
 	pin1 = find_isa_irq_pin(0, mp_INT);
 	pin2 = find_isa_irq_pin(0, mp_ExtINT);

Re: [2.6.4-rc2] bogus semicolon behind if()

[Thomas Schlichter]

Am Freitag, 19. März 2004 21:30 schrieb Maciej W. Rozycki:
> On Fri, 19 Mar 2004, Thomas Schlichter wrote:
> > Well, my timer interrupt goes through the IO-APIC but I do have a
> > functional TSC. Nevertheless my system requires timer_ack to be set... If
> > it isn't, my CPU does not utilize its C2 state...
>
>  Hmm, I wonder if there's any relationship between the state of the local
> APIC and your observation.  Can you please see if the following hack
> changes anything (this assumes you have your timer IRQ directly connected
> to an I/O APIC input)?

I had to apply this hack by hand as your line numbers don't match mine (I use 
2.6.4-mm2) but I' sorry, this hack doesn't change anything for me... ;-(

   Thomas

Re: [2.6.4-rc2] bogus semicolon behind if()

[Andrew Morton]

Thomas Schlichter <thomas.schlichter@web.de> wrote:
>
> P.S.: Wouldn't it be nice if gcc complained about these mistakes?

It does, with -W.  But -W creates vast amounts of less useful warnings.

Re: [2.6.4-rc2] bogus semicolon behind if()

[Thomas Schlichter]

Am Dienstag, 9. März 2004 01:33 schrieb Andrew Morton:
> Thomas Schlichter <thomas.schlichter@web.de> wrote:
> > P.S.: Wouldn't it be nice if gcc complained about these mistakes?
>
> It does, with -W.  But -W creates vast amounts of less useful warnings.

Ah, OK... Thanks!

Re: [2.6.4-rc2] bogus semicolon behind if()

[Mikael Pettersson]

On Thu, 18 Mar 2004 01:52:36 -0800, Andrew Morton wrote:
>Mikael Pettersson <mikpe@csd.uu.se> wrote:
>>
>> Maciej W. Rozycki writes:
>>  > On Wed, 17 Mar 2004, Andrew Morton wrote:
>>  > 
>>  > > I still have a couple of NMI patches in -mm:
>>  > > 
>>  > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi_watchdog-local-apic-fix.patch
>>  > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5-rc1/2.6.5-rc1-mm1/broken-out/nmi-1-hz.patch
>>  > > 
>>  > > What should we do with these?
>>  > 
>>  >  I think we should ask Mikael Pettersson as he is the local APIC watchdog 
>>  > expert.  Mikael?
>> 
>> Will do. Is there a problem with them, or do you just want them
>> reviewed for merging into 2.6.5-rc?
>> 
>
>They seem to work OK - I did a batch of testing with various setups.  But a
>retest wouldn't hurt.
>
>We mainly need a review and general finish-it-off-and-bless-it please.

'nmi_watchdog-local-apic-fix.patch' looks Ok, although I
would prefer if it didn't put a 'static' on nmi_perfctr_msr:
the perfctr driver needs access to it.

I'm not happy with 'nmi-1-hz.patch'. The real problem is that
SMP with nmi_watchdog=2 initialises the lapic NMI watchdog but
doesn't check it and therefore doesn't reduce nmi_hz.
This is an SMP bug, but instead of fixing it the patch removes
the check from UP and changes nmi.c to compensate.

This would be Ok if check_nmi_watchdog() with nmi_watchdog=2
was redundant, but I don't believe it is. It has exposed bugs
and unexpected hardware changes before, and so should remain.

I propose the patch below instead. It changes smpboot.c to do a
check_nmi_watchdog() at the appropriate place, which fixes the
high NMI frequency problem w/o changing anything else.
I've verified that it solves the problem on my MP-capable UP box.

/Mikael

diff -ruN linux-2.6.5-rc2/arch/i386/kernel/smpboot.c linux-2.6.5-rc2.smpboot-lapic-watchdog-fix/arch/i386/kernel/smpboot.c
--- linux-2.6.5-rc2/arch/i386/kernel/smpboot.c	2004-03-11 14:01:25.000000000 +0100
+++ linux-2.6.5-rc2.smpboot-lapic-watchdog-fix/arch/i386/kernel/smpboot.c	2004-03-20 17:21:30.113862000 +0100
@@ -1107,6 +1107,9 @@
 		}
 	}
 
+	if (nmi_watchdog == NMI_LOCAL_APIC)
+		check_nmi_watchdog();
+
 	smpboot_setup_io_apic();
 
 	setup_boot_APIC_clock();