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