On Thu, May 31, 2012 at 01:06:57PM +0300, Gleb Natapov wrote:
> On Wed, May 30, 2012 at 05:54:42PM -0300, Marcelo Tosatti wrote:
> > On Tue, May 22, 2012 at 03:54:56PM +0300, Michael S. Tsirkin wrote:
> > > We perform ISR lookups twice: during interrupt
> > > injection and on EOI. Typical workloads only have
> > > a single bit set there. So we can avoid ISR scans by
> > > 1. counting bits as we set/clear them in ISR
> > > 2. if count is 1, caching the vector number
> > > 3. if count != 1, invalidating the cache
> > >
> > > The real purpose of this is enabling PV EOI
> > > which needs to quickly validate the vector.
> > > But non PV guests might also benefit.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > ---
> > >
> > > I am well aware of Thomas and Peter's suggestion of reworking APIC
> > > register handling in kvm instead of adding a cache like this patch does.
> > >
> > > This revision does *not* address that comment yet: it only corrects a
> > > bug in the original patch.
> > >
> > > Posting in this form for ease of testing.
> > >
> > > Changes from v1:
> > > replace ASSERT by BUG_ON, correcting inverted logic
> > >
> > > arch/x86/kvm/lapic.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > > arch/x86/kvm/lapic.h | 2 +
> > > 2 files changed, 51 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 93c1574..0d2985d 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -107,6 +107,16 @@ static inline void apic_clear_vector(int vec, void *bitmap)
> > > clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > }
> > >
> > > +static inline int __apic_test_and_set_vector(int vec, void *bitmap)
> > > +{
> > > + return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > +}
> > > +
> > > +static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> > > +{
> > > + return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > +}
> > > +
> > > static inline int apic_hw_enabled(struct kvm_lapic *apic)
> > > {
> > > return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> > > @@ -210,6 +220,16 @@ static int find_highest_vector(void *bitmap)
> > > return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
> > > }
> > >
> > > +static u8 count_vectors(void *bitmap)
> > > +{
> > > + u32 *word = bitmap;
> > > + int word_offset;
> > > + u8 count = 0;
> > > + for (word_offset = 0; word_offset < MAX_APIC_VECTOR >> 5; ++word_offset)
> > > + count += hweight32(word[word_offset << 2]);
> > > + return count;
> > > +}
> > > +
> > > static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
> > > {
> > > apic->irr_pending = true;
> > > @@ -242,6 +262,25 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
> > > apic->irr_pending = true;
> > > }
> > >
> > > +static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> > > +{
> > > + if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> > > + ++apic->isr_count;
> > > + BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> > > + if (likely(apic->isr_count == 1))
> > > + apic->isr_cache = vec;
> > > + else
> > > + apic->isr_cache = -1;
> > > +}
> > > +
> > > +static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> > > +{
> > > + if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> > > + --apic->isr_count;
> > > + BUG_ON(apic->isr_count < 0);
> > > + apic->isr_cache = -1;
> > > +}
> > > +
> > > int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> > > {
> > > struct kvm_lapic *apic = vcpu->arch.apic;
> > > @@ -273,6 +312,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
> > > static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > {
> > > int result;
> > > + if (!apic->isr_count)
> > > + return -1;
> > > + if (likely(apic->isr_cache != -1))
> >
> > assert(isr_count == 1).
> >
> > Looks fine otherwise. Gleb can you review please?
> I am not convinced we need to keep track of isr_count. isr_cache should be enough.
> Why setting isr_cache to -1 if isr_count > 1?
So that data path can check cache and if != -1 know it
is valid: we check the cache more than we set it.
I'll add a comment to explain this.
> Just overwrite isr_cache
> values with newly injected vector and use it on next EOI.
> --
> 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
