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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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.

> > 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. lapic changes should be minimal.

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

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

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


[KVM ARM]     [KVM ia64]     [KVM ppc]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Linux Kernel]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux