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

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

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

Sheng Yang wrote:
> On Fri, Oct 17, 2008 at 07:40:00PM +0200, Jan Kiszka wrote:
>> 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).
> 
> Yeah, I am agree on this point now. But for the above one, NO... :)
> 
> Using apic_local_deliver() also means I ignored PIC and make it transpent.
> Please don't involve it in again. It's *not* the normal usage. I want to
> keep the impact as small as possible.

??? You lost me again. Are we still talking about the changes of this
particular patch? In what way does it "involve" apic_local_deliver
again? And into what?

Jan


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