Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing

[Gleb Natapov <gleb@redhat.com>]

On Sun, Jun 02, 2013 at 04:32:25PM +0200, Paolo Bonzini wrote:
> > So what I didn't like from the start about
> > pending_events is that it introduces two locked instruction on each
> > interrupt injection path, your patch makes it worse by change one of
> > those locked instruction to cmpxchg, while mine actually removes one.
> 
> A cmpxchg is not more expensive than a test_and_clear_bit.  A cmpxchg
> loop would be worse, of course.
> 
Not sure about it. cmpxchg is regarded to be expensive AFAIK, but of
course how expensive depends on particular cpu.

> > But I think we can do even better and get rid of both of them for common
> > case and do only one locked inst while there are events pending, but
> > this is slow path so less important: 
> 
> It looks indeed better than both alternatives.  It doesn't do the
> coalescing that worries you, and I can understand it relatively easily
> as "latching" the contents of pending_events at the beginning of
> kvm_apic_accept_events.  Very good idea!
> 
We can do the same for event processing during vcpu entry and remove a
lot of locking instruction from there.

> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 9d75193..3e0e85a 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1850,11 +1850,14 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_lapic *apic = vcpu->arch.apic;
> >  	unsigned int sipi_vector;
> > +	unsigned long pe;
> >  
> > -	if (!kvm_vcpu_has_lapic(vcpu))
> > +	if (!kvm_vcpu_has_lapic(vcpu) || !apic->pending_events)
> >  		return;
> 
> FWIW, this optimization is independent of the other change.  It would
> work even on top of the current code.  But of course there is no need to
> split it into a separate patch.
> 
Agree.

> Paolo
> 
> > -	if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
> > +	pe = xchg(&apic->pending_events, 0);
> > +
> > +	if (test_bit(KVM_APIC_INIT, &pe)) {
> >  		kvm_lapic_reset(vcpu);
> >  		kvm_vcpu_reset(vcpu);
> >  		if (kvm_vcpu_is_bsp(apic->vcpu))
> > @@ -1862,7 +1865,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
> >  		else
> >  			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> >  	}
> > -	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
> > +	if (test_bit(KVM_APIC_SIPI, &pe) &&
> >  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> >  		/* evaluate pending_events before reading the vector */
> >  		smp_rmb();
> > --
> > 			Gleb.
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

--
			Gleb.