Re: [PATCHv2] kvm: optimize ISR lookups

[Michael S. Tsirkin]

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? Just overwrite isr_cache
> values with newly injected vector and use it on next EOI.

I thought about this some more. This counter is a win because
apic_find_highest_isr is called twice:

	1. apic_set_eoi calls apic_find_highest_isr before clearing

	2. apic_update_ppr calls apic_find_highest_isr after clearing

So with isr_count you never scan ISR unless there is nesting.

So we do need isr_count. I agree we can make this work for
more cases because the latest ISR set is always the
highest one. Ronen suggested such an
optimization offline too, and it makes
code simpler so I'll implement this optimization.

> --
> 			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/