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

["Michael S. Tsirkin" <mst@redhat.com>]

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.

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

> lapic changes should be minimal.

Exactly my motivation.

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

> > 
> > > 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?
This would certainly add some overhead.

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.

> --
> 			Gleb.