Re: [PATCH 2/2] x86/crash: Disable the watchdog NMIs on the crashing cpu.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/11/13 10:33, Andrew Cooper wrote:
> On 18/11/13 09:26, Jan Beulich wrote:
>>>>> On 15.11.13 at 21:32, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/crash.c
>>> +++ b/xen/arch/x86/crash.c
>>> @@ -118,6 +118,7 @@ static void nmi_shootdown_cpus(void)
>>>      unsigned long msecs;
>>>      int i, cpu = smp_processor_id();
>>>  
>>> +    disable_lapic_nmi_watchdog();
>>>      local_irq_disable();
>>>  
>>>      crashing_cpu = cpu;
>> _If_ you do this here, I wonder why it's being done before
>> disabling interrupts.
>>
>> But then again I wonder whether it wouldn't be better to do
>> this even earlier (i.e. by passing a flag to watchdog_disable()),
>> as the NMI watchdog becomes useless with that call being done
>> from kexec_common_shutdown().
> Disabling interrupts here is more defensive coding than anything else. 
> It is not expected to be able to get here with interrupts enabled, but
> in a crash

... we can make no guarantees.

>
> Putting this in watchdog_disable() would result in a race condition. 
> disable_lapic_nmi_watchdog() mutates global state, meaning that it can
> only possibly run correctly on a single cpu. In an ideal world with
> plenty of time, the lapic watchdog code could be improved.  However,
> restricting its use until after one_cpu_only() is the easiest fix.
>
> ~Andrew
>
>>> --- a/xen/arch/x86/nmi.c
>>> +++ b/xen/arch/x86/nmi.c
>>> @@ -165,7 +165,7 @@ static void nmi_timer_fn(void *unused)
>>>      set_timer(&this_cpu(nmi_timer), NOW() + MILLISECS(1000));
>>>  }
>>>  
>>> -static void disable_lapic_nmi_watchdog(void)
>>> +void disable_lapic_nmi_watchdog(void)
>> The suggested alternative would also make it unnecessary to
>> make this function non-static...
>>
>> Jan
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel