On Mon, Apr 16, 2012 at 06:10:11PM +0300, Gleb Natapov wrote: > On Mon, Apr 16, 2012 at 04:13:29PM +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 16, 2012 at 03:30:47PM +0300, Gleb Natapov wrote: > > > On Mon, Apr 16, 2012 at 03:18:25PM +0300, Michael S. Tsirkin wrote: > > > > On Mon, Apr 16, 2012 at 02:24:46PM +0300, Gleb Natapov wrote: > > > > > On Mon, Apr 16, 2012 at 02:09:20PM +0300, Michael S. Tsirkin wrote: > > > > > > Thanks very much for the review. I'll address the comments. > > > > > > Some questions on your comments below. > > > > > > > > > > > > On Mon, Apr 16, 2012 at 01:08:07PM +0300, Gleb Natapov wrote: > > > > > > > > @@ -37,6 +38,8 @@ > > > > > > > > #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01 > > > > > > > > #define MSR_KVM_ASYNC_PF_EN 0x4b564d02 > > > > > > > > #define MSR_KVM_STEAL_TIME 0x4b564d03 > > > > > > > > +#define MSR_KVM_EOI_EN 0x4b564d04 > > > > > > > > +#define MSR_KVM_EOI_DISABLED 0x0L > > > > > > > This is valid gpa. Follow others MSR example i.e align the address to, > > > > > > > lets say dword, and use lsb as enable bit. > > > > > > > > > > > > We only need a single byte, since this is per-CPU - > > > > > > it's better to save the memory, so no alignment is required. > > > > > > An explicit disable msr would also address this, right? > > > > > > > > > > > We do not have shortage of memory. > > > > > Better make all MSRs works the same > > > > > way. > > > > > > > > I agree it's nice to have EOI and ASYNC_PF look similar > > > > but wasting memory is also bad. I'll ponder this some more. > > > > > > > Steal time and kvm clock too and may be others (if anything left at > > > all). I hope you are kidding about wasting of 4 bytes per vcpu. > > > > Not vcpu - cpu. It's wasted whenever kernel/kvm.c is built so it has > > cost on physical machines as well. > > > There are less real cpus than vcpus usually :) I'm adding this percpu always. This makes it cheap to access but it means it is allocated on physical cpus - just unused there. > > > > > BTW have you added new MSR to msrs_to_save array? I forgot to > > > > > checked. > > > > > > > > I didn't yet. Trying to understand how will that affect > > > > cross-version migration - any input? > > > > > > > Not sure. You need to check what userspace does with them. > > > > > > > > > > > +static void apic_update_isr(struct kvm_lapic *apic) > > > > > > > > +{ > > > > > > > > + int vector; > > > > > > > > + if (!eoi_enabled(apic->vcpu) || > > > > > > > > + !apic->vcpu->arch.eoi.pending || > > > > > > > > + eoi_get_pending(apic->vcpu)) > > > > > > > > + return; > > > > > > > > + apic->vcpu->arch.eoi.pending = false; > > > > > > > > + vector = apic_find_highest_isr(apic); > > > > > > > > + if (vector == -1) > > > > > > > > + return; > > > > > > > > + apic_clear_vector(vector, apic->regs + APIC_ISR); > > > > > > > > +} > > > > > > > > + > > > > > > > We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). > > > > > > > This removes the need for the function and its calls. > > > > > > > > > > > > It's a bit of a waste: that one does all kind extra things > > > > > > which we know we don't need, some of the atomics. And it's datapath > > > > > > so extra stuff is not free. > > > > > > > > > > > How much time those extra things are taking compared to vmexit you > > > > > already serving? And there is a good chance you will do them during > > > > > vmentry anyway while trying to inject (or just check for) new interrupt. > > > > > > > > No need to do them twice :) > > > > > > > > > > Probably a good idea to replace the call on MSR disable - I think > > > > > > apic_update_ppr is a better thing to call there. > > > > > > > > > > > > Is there anything else that I missed? > > > > > I think that simple things are better then complex things if the end result is > > > > > the same :) Try it and see how much simpler it is. > > > > > > > > It doesn't seem to be simpler at all. The common functionality is > > > > about 4 lines. > > > Send patch for us to see. > > > > That's what you are replying to, no? > > You can see that it is 4 lines of code. > No. I mean something like patch below. Applies on top of yours. Did not > check that it works or even compiles. > > > > > > lapic changes should be minimal. > > > > Exactly my motivation. > > > My patch removes 13 lines more :) I'll take a look, thanks. > > > > > > > > > Have you measured > > > > > that what you are trying to optimize actually worth optimizing? That you > > > > > can measure the optimization at all? > > > > > > > > The claim is not that it's measureable. The claim is that > > > > it does not scale to keep adding things to do on each entry. > > > > > > > Only if there is something to do. "Premature optimization is the root of > > > all evil". The PV eoi is about not exiting on eoi unnecessary. You are > > > mixing this with trying to avoid calling eoi code for given interrupt at > > > all. > > > > I don't think this is what my patch does. EOI still clears ISR > > for each interrupt. > > > > > Two different optimization, do not try lump them together. > > > > > > > > > > > > > We already have > > > > > > > call to kvm_lapic_sync_from_vapic() on exit path which should be > > > > > > > extended to do the above. > > > > > > > > > > > > It already does this. It calls apic_set_tpr > > > > > > which calls apic_update_ppr which calls > > > > > > apic_update_isr. > > > > > > > > > > > It does it only if vapic is in use (and it is usually not). > > > > > > > > When it's not we don't need to update ppr and so > > > > no need to update isr on this exit. > > > If there was eoi we need to update both. > > > > By same logic we should call update_ppr on each entry. > > The overhead is unlikely to be measureable either :). > > > It is small enough for us to not care about it on RHEL6 where it is > called on each entry. Exactly. So why do not we do it? > > > > > > > > > But the if() > > > > > is already there so we do not need to worry that one additional if() on > > > > > the exit path will slow KVM to the crawl. > > > > > > > > The number of things we need to do on each entry keeps going up, if we > > > > just keep adding stuff it won't end well. > > > > > > > You do not add stuff. The if() is already there. > > > > > > Your proposal was to check userspace eoi record > > each time when eoi is pending, no? > Yes. > > > This would certainly add some overhead. > > > Only when eoi is pending. This is rare. This is exactly while guest handles an interrupt. It's not all that rare at all: e.g. device drivers cause an exit from interrupt handler by doing io. > > I also find the logic easier to follow as is - > > it is contained in lapic.c without relying > > on being called from x86.c as just the right moment. > > > See the patch. It change nothing outside of lapic.c. I'll take a look, thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html