Re: [Xen-devel] [PATCH] x86: show remote CPU state upon fatal NMI

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/06/16 08:55, Jan Beulich wrote:
>>> @@ -570,6 +600,15 @@ void fatal_trap(const struct cpu_user_re
>>>              printk("Faulting linear address: %p\n", _p(cr2));
>>>              show_page_walk(cr2);
>>>          }
>>> +        else if ( trapnr == TRAP_nmi )
>>> +        {
>>> +            cpumask_andnot(&nmi_show_state_mask, &cpu_online_map,
>>> +                           cpumask_of(smp_processor_id()));
>>> +            set_nmi_callback(nmi_show_execution_state);
>>> +            smp_send_nmi_allbutself();
>> This would cause far less spinlock contention as
>>
>> for_each_cpu( cpu, nmi_show_state_mask )
>>     smp_send_nmi(cpu);
>>
>> I realise this involves introducing a new smp function, but it would
>> substantially reduce contention on the console lock.
> Again, I don't see why lock contention would matter here. And then
> I also don't see how sending the IPIs individually would make matters
> significantly better: The sending will surely finish much faster than
> the printing.

Contention is a problem because you have replaced the NMI callback, and
the watchdog is still running.  Especially if sync_console is in effect,
you are liable to incur a further timeout, queueing up more NMIs.

Although now I think of it, that won't matter so long as the NMIs don't
nest.

The one advantage of sending the NMIs in order is that the information
dump will happen in order, which is slightly more use than having them
in a random order on a large machine.

>
>> I would recommend moving this clause into nmi_watchdog_tick(), so that
>> it doesn't get involved for non-watchdog NMIs.  IOCK/SERR NMIs won't
>> have anything interesting to print from here.  I would also recommend
>> disabling the watchdog before IPI'ing.
> And indeed I would have wanted it there, but I can't see how it can
> reasonably be put there: fatal_trap() doesn't return, so we can't put
> it after. And we definitely want to get state of the local CPU out
> before we try to log state of any of the remote CPUs. So the only
> options I see would be to
> - somehow specially flag the regs structure, but that feels hackish
>   (among other aspects nmi_watchdog_tick() has that parameter
>   const qualified for the very reason that it isn't supposed to fiddle
>   with it),
> - introduce a global flag.

How about a boolean flag to fatal_trap()?  It doesn't have many callers,
and this kind of printing might also be useful for some MCEs.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel