nmi_watchdog suspicious

nmi_watchdog suspicious

[Cyrill Gorcunov]

Ingo, Maciej, I've a really bad suspicious about commit

---
commit 3ed3f06295e69700fa808396f7b350bff2b69de0
Author: Cyrill Gorcunov <gorcunov@gmail.com>
Date:   Wed Jun 4 01:00:47 2008 +0400

    x86: nmi - consolidate nmi_watchdog_default for 32bit mode
    
    64bit mode bootstrap code does set nmi_watchdog to NMI_NONE
    by default and doing the same on 32bit mode is safe too.
    Such an action saves us from several #ifdef.
    
    Btw, my previous commit
    
    commit 19ec673ced067316b9732bc6d1c4ff4052e5f795
    Author: Cyrill Gorcunov <gorcunov@gmail.com>
    Date:   Wed May 28 23:00:47 2008 +0400
    
        x86: nmi - fix incorrect NMI watchdog used by default
    
    did not fix the problem completely, moreover it
    introduced additional bug - nmi_watchdog would be
    set to either NMI_LOCAL_APIC or NMI_IO_APIC
    _regardless_ to boot option if being enabled thru
    /proc/sys/kernel/nmi_watchdog. Sorry for that.
    Fix it too.
---

actually it changed nmi behaviour much more then I thought.
Look, since by default nmi_watchdog now becomes NMI_NONE it
does mean nmi.c:touch_nmi_watchdog() will not reset apic timers
anymore even being requested by some other code.

This commit does affect 32bit mode _only_. So now I'm guessing
what mode was implemented in correct way originally.

On 64bit mode nmi_watchdog=NMI_NONE by default (in case if APIC enabled).
On 32bit mode nmi_watchdog=NMI_DEFAULT was by default (in any case,
but could be set to NMI_NONE in check_timer(), but we don't take
this case now).

So lets take a look on touch_nmi_watchdog().
There is the condition

	if (nmi_watchdog > 0)
		...tell to reset counters in nmi_watchdog_tick()

this condition is not taken on 64bit mode, but *was* taken on
32bit mode by default! So who was right then? 64bit version or 32bit?

Maciej, could you take a look please? Maybe I just missing figure
in general - ie how nmi_watchdog _should_ work.

		- Cyrill -

Re: nmi_watchdog suspicious

[Cyrill Gorcunov]

[Cyrill Gorcunov - Tue, Jun 10, 2008 at 10:57:59PM +0400]
| Ingo, Maciej, I've a really bad suspicious about commit
| 
| ---
| commit 3ed3f06295e69700fa808396f7b350bff2b69de0
| Author: Cyrill Gorcunov <gorcunov@gmail.com>
| Date:   Wed Jun 4 01:00:47 2008 +0400
| 
|     x86: nmi - consolidate nmi_watchdog_default for 32bit mode
|     
|     64bit mode bootstrap code does set nmi_watchdog to NMI_NONE
|     by default and doing the same on 32bit mode is safe too.
|     Such an action saves us from several #ifdef.
|     
|     Btw, my previous commit
|     
|     commit 19ec673ced067316b9732bc6d1c4ff4052e5f795
|     Author: Cyrill Gorcunov <gorcunov@gmail.com>
|     Date:   Wed May 28 23:00:47 2008 +0400
|     
|         x86: nmi - fix incorrect NMI watchdog used by default
|     
|     did not fix the problem completely, moreover it
|     introduced additional bug - nmi_watchdog would be
|     set to either NMI_LOCAL_APIC or NMI_IO_APIC
|     _regardless_ to boot option if being enabled thru
|     /proc/sys/kernel/nmi_watchdog. Sorry for that.
|     Fix it too.
| ---
| 
| actually it changed nmi behaviour much more then I thought.
| Look, since by default nmi_watchdog now becomes NMI_NONE it
| does mean nmi.c:touch_nmi_watchdog() will not reset apic timers
| anymore even being requested by some other code.
| 
| This commit does affect 32bit mode _only_. So now I'm guessing
| what mode was implemented in correct way originally.
| 
| On 64bit mode nmi_watchdog=NMI_NONE by default (in case if APIC enabled).
| On 32bit mode nmi_watchdog=NMI_DEFAULT was by default (in any case,
| but could be set to NMI_NONE in check_timer(), but we don't take
| this case now).
| 
| So lets take a look on touch_nmi_watchdog().
| There is the condition
| 
| 	if (nmi_watchdog > 0)
| 		...tell to reset counters in nmi_watchdog_tick()
| 
| this condition is not taken on 64bit mode, but *was* taken on
| 32bit mode by default! So who was right then? 64bit version or 32bit?
| 
| Maciej, could you take a look please? Maybe I just missing figure
| in general - ie how nmi_watchdog _should_ work.
| 
| 		- Cyrill -

btw, since in general we support nmi_watchdog for two choises:
lapic and io-apic - i think about to change this condition to

	if (nmi_watchdog == NMI_LOCAL_APIC ||
		nmi_watchdog == NMI_IO_APIC) {

but still in doubts...

		- Cyrill -

Re: nmi_watchdog suspicious

[Maciej W. Rozycki]

On Tue, 10 Jun 2008, Cyrill Gorcunov wrote:

> On 64bit mode nmi_watchdog=NMI_NONE by default (in case if APIC enabled).
> On 32bit mode nmi_watchdog=NMI_DEFAULT was by default (in any case,
> but could be set to NMI_NONE in check_timer(), but we don't take
> this case now).

 I haven't tracked the 64-bit port, but for plain i386 the watchdog used
to be on by default, then proved problematic with too many broken pieces
of equipment (typically because of bugs in the SMM firmware) and thus set
to off.

> So lets take a look on touch_nmi_watchdog().
> There is the condition
> 
> 	if (nmi_watchdog > 0)
> 		...tell to reset counters in nmi_watchdog_tick()
> 
> this condition is not taken on 64bit mode, but *was* taken on
> 32bit mode by default! So who was right then? 64bit version or 32bit?
> 
> Maciej, could you take a look please? Maybe I just missing figure
> in general - ie how nmi_watchdog _should_ work.

 Well, values >= NMI_INVALID are never used, so the condition is correct.  
It is meant to be positive whenever a working watchdog has been selected.  
Obviously nmi_watchdog should be a signed int though, so there is a bug
there.  You better audit all the uses of the variable...

  Maciej

Re: nmi_watchdog suspicious

[Cyrill Gorcunov]

[Maciej W. Rozycki - Mon, Jun 16, 2008 at 12:49:00AM +0100]
| On Tue, 10 Jun 2008, Cyrill Gorcunov wrote:
| 
| > On 64bit mode nmi_watchdog=NMI_NONE by default (in case if APIC enabled).
| > On 32bit mode nmi_watchdog=NMI_DEFAULT was by default (in any case,
| > but could be set to NMI_NONE in check_timer(), but we don't take
| > this case now).
| 
|  I haven't tracked the 64-bit port, but for plain i386 the watchdog used
| to be on by default, then proved problematic with too many broken pieces
| of equipment (typically because of bugs in the SMM firmware) and thus set
| to off.
| 
| > So lets take a look on touch_nmi_watchdog().
| > There is the condition
| > 
| > 	if (nmi_watchdog > 0)
| > 		...tell to reset counters in nmi_watchdog_tick()
| > 
| > this condition is not taken on 64bit mode, but *was* taken on
| > 32bit mode by default! So who was right then? 64bit version or 32bit?
| > 
| > Maciej, could you take a look please? Maybe I just missing figure
| > in general - ie how nmi_watchdog _should_ work.
| 
|  Well, values >= NMI_INVALID are never used, so the condition is correct.  
| It is meant to be positive whenever a working watchdog has been selected.  
| Obviously nmi_watchdog should be a signed int though, so there is a bug
| there.  You better audit all the uses of the variable...
| 
|   Maciej
| 

Maciej, I think nmi_watchdog could (and probably should) be defined as
unsigned. Here my points of why (fix me please if I'm wrong):

- if we remain it as unsigned we could simplify setup_nmi_watchdog() to
  just check for 'if (nmi >= NMI_INVALID)'
- current code does check for NMI_NONE _and_ NMI_DISABLED at once in most
  cases (only the case it dont is - proc_nmi_enabled() wich could be simplified too)
- the only affected of such sign/unsign contention I found is
  touch_nmi_watchdog() for which I suggested the patch (already in Ingo's tip tree)
  http://lkml.org/lkml/2008/6/12/200
  So there could be some 'useless counters resetting' but it could happen for
  quite short time while APIC in initialization phase.

So I think the only problem could be is - simplification. Maybe some checks
should be isolated in helper functions. Will take a look (and of course will
keep community in touch ;)

		- Cyrill -

Re: nmi_watchdog suspicious

[Maciej W. Rozycki]

On Mon, 16 Jun 2008, Cyrill Gorcunov wrote:

> Maciej, I think nmi_watchdog could (and probably should) be defined as
> unsigned. Here my points of why (fix me please if I'm wrong):
> 
> - if we remain it as unsigned we could simplify setup_nmi_watchdog() to
>   just check for 'if (nmi >= NMI_INVALID)'

 This is run once only at the boot if at all -- just to verify the range
is correct.  Other places are executed multiple times during normal
operation and it is them you should optimise for.

> - current code does check for NMI_NONE _and_ NMI_DISABLED at once in most
>   cases (only the case it dont is - proc_nmi_enabled() wich could be simplified too)

 Please note the intent is NMI_DISABLED is a bootstrap default to tell the
platform the user has not specified any override.  With the 32-bit
platform it used to be promoted automatically to NMI_IO_APIC or
NMI_LOCAL_APIC as appropriate, but it was removed because of stability
problems with many systems.  It looks it wasn't done in a particularly
fortunate way -- the new promotion should be to NMI_NONE, but instead it
was removed altogether.

 Preferably the initialization to NMI_NONE should be done as soon as it
has been determined there was no "nmi_watchdog=" option specified, but in
practice I think it can simply be done at the beginning of trap_init(),
before the gate descriptor has been set up for the NMI (after which point
the NMI handler can be reached).  This way no piece of code other than
setup_nmi_watchdog() would have to care about negative values of
nmi_watchdog.

> - the only affected of such sign/unsign contention I found is
>   touch_nmi_watchdog() for which I suggested the patch (already in Ingo's tip tree)
>   http://lkml.org/lkml/2008/6/12/200
>   So there could be some 'useless counters resetting' but it could happen for
>   quite short time while APIC in initialization phase.

 This is a sloppy coding practice which has led us to the current
situation with the APIC code -- there should be no "useless code
execution" unless absolutely unavoidable.  I'd feel more comfortable if
there was a separate variable like nmi_watchdog_active checked in the
handler instead of nmi_watchdog that would only be set once the watchdog
has actually been activated.

 The whole idea of touch_nmi_watchdog() itself is rather unfortunate too,
but that's apparently not an easy problem to solve.

  Maciej

Re: nmi_watchdog suspicious

[Cyrill Gorcunov]

[Maciej W. Rozycki - Tue, Jun 17, 2008 at 12:20:28AM +0100]
| On Mon, 16 Jun 2008, Cyrill Gorcunov wrote:
| 
| > Maciej, I think nmi_watchdog could (and probably should) be defined as
| > unsigned. Here my points of why (fix me please if I'm wrong):
| > 
| > - if we remain it as unsigned we could simplify setup_nmi_watchdog() to
| >   just check for 'if (nmi >= NMI_INVALID)'
| 
|  This is run once only at the boot if at all -- just to verify the range
| is correct.  Other places are executed multiple times during normal
| operation and it is them you should optimise for.
| 
| > - current code does check for NMI_NONE _and_ NMI_DISABLED at once in most
| >   cases (only the case it dont is - proc_nmi_enabled() wich could be simplified too)
| 
|  Please note the intent is NMI_DISABLED is a bootstrap default to tell the
| platform the user has not specified any override.  With the 32-bit
| platform it used to be promoted automatically to NMI_IO_APIC or
| NMI_LOCAL_APIC as appropriate, but it was removed because of stability
| problems with many systems.  It looks it wasn't done in a particularly
| fortunate way -- the new promotion should be to NMI_NONE, but instead it
| was removed altogether.
| 
|  Preferably the initialization to NMI_NONE should be done as soon as it
| has been determined there was no "nmi_watchdog=" option specified, but in
| practice I think it can simply be done at the beginning of trap_init(),
| before the gate descriptor has been set up for the NMI (after which point
| the NMI handler can be reached).  This way no piece of code other than
| setup_nmi_watchdog() would have to care about negative values of
| nmi_watchdog.
| 
| > - the only affected of such sign/unsign contention I found is
| >   touch_nmi_watchdog() for which I suggested the patch (already in Ingo's tip tree)
| >   http://lkml.org/lkml/2008/6/12/200
| >   So there could be some 'useless counters resetting' but it could happen for
| >   quite short time while APIC in initialization phase.
| 
|  This is a sloppy coding practice which has led us to the current
| situation with the APIC code -- there should be no "useless code
| execution" unless absolutely unavoidable.  I'd feel more comfortable if
| there was a separate variable like nmi_watchdog_active checked in the
| handler instead of nmi_watchdog that would only be set once the watchdog
| has actually been activated.
| 
|  The whole idea of touch_nmi_watchdog() itself is rather unfortunate too,
| but that's apparently not an easy problem to solve.
| 
|   Maciej
|

Thanks a lot Maciej for comments! I've marked them. I'm not sure but it seems
I wrote a bit unclear /my english bad indeed/ ;) I mean - this say 'slipping'
(ie useless code executions) _was_ before the patch applied. Now it doesn't
slip on this since we do mention explicitly in which case there should be
alert counters reset. Other then that - will try to handle your notes. Thanks!

		- Cyrill -

Re: nmi_watchdog suspicious

[Maciej W. Rozycki]

On Tue, 17 Jun 2008, Cyrill Gorcunov wrote:

> Thanks a lot Maciej for comments! I've marked them. I'm not sure but it seems
> I wrote a bit unclear /my english bad indeed/ ;) I mean - this say 'slipping'
> (ie useless code executions) _was_ before the patch applied. Now it doesn't
> slip on this since we do mention explicitly in which case there should be
> alert counters reset. Other then that - will try to handle your notes. Thanks!

 It will happen regardless if touch_nmi_watchdog() is called before the
NMI watchdog has been set up in setup_nmi() or lapic_watchdog_init().  It
may also happen during that window if an NMI is signalled without any NMI
status bits set in the Port B register at 0x61 -- in theory that should
not happen except for a broken configuration, but reality out there seems
to be quite creative about breakage.

  Maciej

Re: nmi_watchdog suspicious

[Cyrill Gorcunov]

[Maciej W. Rozycki - Wed, Jun 18, 2008 at 05:06:29PM +0100]
| On Tue, 17 Jun 2008, Cyrill Gorcunov wrote:
| 
| > Thanks a lot Maciej for comments! I've marked them. I'm not sure but it seems
| > I wrote a bit unclear /my english bad indeed/ ;) I mean - this say 'slipping'
| > (ie useless code executions) _was_ before the patch applied. Now it doesn't
| > slip on this since we do mention explicitly in which case there should be
| > alert counters reset. Other then that - will try to handle your notes. Thanks!
| 
|  It will happen regardless if touch_nmi_watchdog() is called before the
| NMI watchdog has been set up in setup_nmi() or lapic_watchdog_init().  It
| may also happen during that window if an NMI is signalled without any NMI
| status bits set in the Port B register at 0x61 -- in theory that should
| not happen except for a broken configuration, but reality out there seems
| to be quite creative about breakage.
| 
|   Maciej
| 

Maciej, it seems we are talking about different code snippets ;)
I'm talking only about touch_nmi_watchdog(). By now (in -tip tree
we have)

void touch_nmi_watchdog(void)
{
	if (nmi_watchdog == NMI_LOCAL_APIC ||
		nmi_watchdog == NMI_IO_APIC) {
		unsigned cpu;
		...

so we check explicitly the values (so if touch_nmi_watchdog
was called when nmi_watchdog = 0 or -1U this code will not
be executed anyway). So I think I'm a bit lost, Maciej... I just
can't figure out what is wrong with this code, so please help
me ;). If you're talking about apic code in _general_ design
then...well, I think I need some time to _understand_ the code
say byte-by-byte first.

		- Cyrill -

Re: nmi_watchdog suspicious

[Maciej W. Rozycki]

On Wed, 18 Jun 2008, Cyrill Gorcunov wrote:

> Maciej, it seems we are talking about different code snippets ;)
> I'm talking only about touch_nmi_watchdog(). By now (in -tip tree
> we have)
> 
> void touch_nmi_watchdog(void)
> {
> 	if (nmi_watchdog == NMI_LOCAL_APIC ||
> 		nmi_watchdog == NMI_IO_APIC) {
> 		unsigned cpu;
> 		...
> 
> so we check explicitly the values (so if touch_nmi_watchdog
> was called when nmi_watchdog = 0 or -1U this code will not
> be executed anyway). So I think I'm a bit lost, Maciej... I just
> can't figure out what is wrong with this code, so please help
> me ;). If you're talking about apic code in _general_ design
> then...well, I think I need some time to _understand_ the code
> say byte-by-byte first.

 The value of nmi_watchdog being NMI_IO_APIC or NMI_LOCAL_APIC does not 
mean the watchdog has been set up already.  This observation applies both 
here and elsewhere, e.g. to nmi_watchdog_tick().

  Maciej

Re: nmi_watchdog suspicious

[Cyrill Gorcunov]

[Maciej W. Rozycki - Wed, Jun 18, 2008 at 05:54:32PM +0100]
| On Wed, 18 Jun 2008, Cyrill Gorcunov wrote:
| 
| > Maciej, it seems we are talking about different code snippets ;)
| > I'm talking only about touch_nmi_watchdog(). By now (in -tip tree
| > we have)
| > 
| > void touch_nmi_watchdog(void)
| > {
| > 	if (nmi_watchdog == NMI_LOCAL_APIC ||
| > 		nmi_watchdog == NMI_IO_APIC) {
| > 		unsigned cpu;
| > 		...
| > 
| > so we check explicitly the values (so if touch_nmi_watchdog
| > was called when nmi_watchdog = 0 or -1U this code will not
| > be executed anyway). So I think I'm a bit lost, Maciej... I just
| > can't figure out what is wrong with this code, so please help
| > me ;). If you're talking about apic code in _general_ design
| > then...well, I think I need some time to _understand_ the code
| > say byte-by-byte first.
| 
|  The value of nmi_watchdog being NMI_IO_APIC or NMI_LOCAL_APIC does not 
| mean the watchdog has been set up already.  This observation applies both 
| here and elsewhere, e.g. to nmi_watchdog_tick().
| 
|   Maciej
|

If you mean the case we get NMI physical line assetred while configuring APIC
(ie nmi watchdog is not properly configured yet) then I wonder why we has
this checking at all...

		- Cyrill -

Re: nmi_watchdog suspicious

[Maciej W. Rozycki]

On Wed, 18 Jun 2008, Cyrill Gorcunov wrote:

> If you mean the case we get NMI physical line assetred while configuring APIC
> (ie nmi watchdog is not properly configured yet) then I wonder why we has
> this checking at all...

 Hmm, I am not sure what you mean -- nmi_watchdog_tick() can be called
potentially at any time, and you can receive NMIs from sources other than
the watchdog; the latters would set bits in the Port B at 0x61 in most
cases though.  Anyway using a separate variable such as
"nmi_watchdog_active" would be cleaner.

  Maciej

Re: nmi_watchdog suspicious

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sat, Jun 21, 2008 at 02:54:38AM +0100]
| On Wed, 18 Jun 2008, Cyrill Gorcunov wrote:
| 
| > If you mean the case we get NMI physical line assetred while configuring APIC
| > (ie nmi watchdog is not properly configured yet) then I wonder why we has
| > this checking at all...
| 
|  Hmm, I am not sure what you mean -- nmi_watchdog_tick() can be called
| potentially at any time, and you can receive NMIs from sources other than
| the watchdog; the latters would set bits in the Port B at 0x61 in most
| cases though.  Anyway using a separate variable such as
| "nmi_watchdog_active" would be cleaner.
| 
|   Maciej
| 

ok, thanks Maciej, i'll take a look on this.

		- Cyrill -