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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 02/06/2013 15:14, Gleb Natapov ha scritto:
>> Can you show what is the case in my patch where you have coalescing?  I
> You'ev said it in some of your emails. Quoting:
> "      INIT-INIT-SIPI-INIT-SIPI
> 
>  your version would do many SIPIs, while mine would do just one."

Cancelling is very different from coalescing.  In the implementation we
chose, we have "two wires" to the processor; an INIT interrupt means
"bring SIPI wire to 0 and INIT to 1", while a SIPI means "leave INIT
wire aside and bring the SIPI wire to 1".  If the target processor does
not sense in time that the SIPI wire is high, the signal is lost.

Coalescing means that INIT-INIT or SIPI-SIPI becomes a single SIPI.
That is happening no matter what, because we have two bits not a queue.

So let's settle this as a simple disagreement on terms and move on.

> There is nothing "surprising" in it for me. Really it is so subjection
> that arguing about it is waste of everybody time and energy. So if we
> want to continue have fun arguing about it lets move to some real patch
> problems/benefits.

Good idea. :)

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

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

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

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
>