Re: [PATCHv1 dont apply] RFC: kvm eoi PV using shared memory

[Gleb Natapov <gleb@redhat.com>]

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. BTW have you added new MSR to msrs_to_save array? I forgot to
checked.

> > > +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.

> 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. Have you measured
that what you are trying to optimize actually worth optimizing? That you
can measure the optimization at all?

> 
> > 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). 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.

--
			Gleb.