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
signature.asc
Description: OpenPGP digital signature
