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

[Gleb Natapov <gleb@redhat.com>]

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

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

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

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

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d184a41..8fb5eca 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -323,20 +323,6 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 	return result;
 }
 
-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);
-}
-
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;
@@ -344,7 +330,6 @@ static void apic_update_ppr(struct kvm_lapic *apic)
 
 	old_ppr = apic_get_reg(apic, APIC_PROCPRI);
 	tpr = apic_get_reg(apic, APIC_TASKPRI);
-	apic_update_isr(apic);
 	isr = apic_find_highest_isr(apic);
 	isrv = (isr != -1) ? isr : 0;
 
@@ -1361,14 +1346,19 @@ void kvm_lapic_sync_from_vapic(struct kvm_vcpu *vcpu)
 	u32 data;
 	void *vapic;
 
-	if (!irqchip_in_kernel(vcpu->kvm) || !vcpu->arch.apic->vapic_addr)
+	if (!irqchip_in_kernel(vcpu->kvm) || !vcpu->arch.apic->vapic_addr ||
+			!apic->vcpu->arch.eoi.pending)
 		return;
 
-	vapic = kmap_atomic(vcpu->arch.apic->vapic_page);
-	data = *(u32 *)(vapic + offset_in_page(vcpu->arch.apic->vapic_addr));
-	kunmap_atomic(vapic);
+	if (apic->vcpu->arch.eoi.pending && !eoi_get_pending(apic->vcpu)) {
+		apic_set_eoi(apic);
+	} else {
+		vapic = kmap_atomic(vcpu->arch.apic->vapic_page);
+		data = *(u32 *)(vapic + offset_in_page(vcpu->arch.apic->vapic_addr));
+		kunmap_atomic(vapic);
 
-	apic_set_tpr(vcpu->arch.apic, data & 0xff);
+		apic_set_tpr(vcpu->arch.apic, data & 0xff);
+	}
 }
 
 void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu)
@@ -1469,13 +1459,10 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 
 int kvm_pv_enable_apic_eoi(struct kvm_vcpu *vcpu, u64 data)
 {
-	if (data == MSR_KVM_EOI_DISABLED) {
-		struct kvm_lapic *apic = vcpu->arch.apic;
-		if (apic && apic_enabled(apic))
-			apic_update_isr(apic);
-	} else if (kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.eoi.data,
-					     data))
-		return 1;
+	if (data != MSR_KVM_EOI_DISABLED)
+		if (kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.eoi.data,
+					data))
+			return 1;
 
 	vcpu->arch.eoi.msr_val = data;
 	vcpu->arch.eoi.pending = false;
--
			Gleb.