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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
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
