From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing
Date: Sun, 2 Jun 2013 20:33:23 +0300
Message-ID: <20130602173323.GO24773@redhat.com>
References: <20130530131054.GB5495@redhat.com>
 <51A752D7.1020809@redhat.com>
 <20130530133534.GC5495@redhat.com>
 <51A75F07.805@redhat.com>
 <20130531043643.GA26250@redhat.com>
 <51A863E0.9080202@redhat.com>
 <20130531091835.GA467@redhat.com>
 <51A871DA.7070905@redhat.com>
 <20130602131442.GF24773@redhat.com>
 <51AB5779.80101@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, Jan Kiszka <jan.kiszka@siemens.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:7647 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754669Ab3FBRd1 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 2 Jun 2013 13:33:27 -0400
Content-Disposition: inline
In-Reply-To: <51AB5779.80101@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.