Re: [PATCH 3/3] KVM: x86: Optimize NMI watchdog delivery

[Jan Kiszka <jan.kiszka@web.de>]

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

Sheng Yang wrote:
> On Fri, Oct 17, 2008 at 07:23:01PM +0200, Jan Kiszka wrote:
>> Sheng Yang wrote:
>>> On Wed, Oct 15, 2008 at 04:27:51PM +0200, Jan Kiszka wrote:
>>>> As suggested by Avi, this patch introduces a counter of VCPUs that have
>>>> LVT0 set to NMI mode. Only if the counter > 0, we push the PIT ticks via
>>>> all LAPIC LVT0 lines to enable NMI watchdog support.
>>>>
>>> I feel a little strange about: if *counter > 0*, we push to *all*. Can we
>>> only push NMIs to the ones that set NMI for LVT0?
>> We don't do that due to !kvm_apic_accept_pic_intr(). The counter is only
>> about optimizing that case where we don't have to walk the whole chain,
>> asking every vcpu if it would like to receive the IRQ.
> 
> I don't agree to use kvm_apic_accept_pic_intr() here, as I explained in the
> first mail. It's not a normal path, and current KVM handle it well.

Current KVM only support PIC Mode, which is fine, but not sufficient for
NMI watchdog support. We need to get the Virtual Wire Mode in, but
correctly.

>>> How about add a field in struct kvm_lapic? We can quickly know if we need to
>>> inject NMI for this vcpu. Well, though kernel mostly enable NMI watchdog on
>>> all vcpu, I think this is more precise, and match the logic, and avoid one
>>> more field in kvm_arch...
>> The point of this patch is to avoid touching vcpu structures AT ALL when
>> there is no interest in the NMI watchdog (normally, OSes will either
>> enable the WD trick for all CPUSs or keep it off).
> 
> Logically, I think lapic is more proper place. And put a bool there won't
> affect much. I think we can do it more straightly here.

If you have dozens of lapics, you don't want to check them all if they
are ALL switched of anyway. That information is better encoded in a
single, (virtual) system-wide bool. That's the most common case we want
to speed up. And it is the core of the optimization Avi suggested
(unless I totally misunderstood him).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]