Re: [PATCH 1/5] KVM: x86: avoid atomic operations on APICv vmentry

[Radim KrÄmÃÅ]

2016-10-27 00:42+0300, Michael S. Tsirkin:
> On Wed, Oct 26, 2016 at 09:53:45PM +0200, Radim KrÄmÃÅ wrote:
>> 2016-10-14 20:21+0200, Paolo Bonzini:
>> > On some benchmarks (e.g. netperf with ioeventfd disabled), APICv
>> > posted interrupts turn out to be slower than interrupt injection via
>> > KVM_REQ_EVENT.
>> > 
>> > This patch optimizes a bit the IRR update, avoiding expensive atomic
>> > operations in the common case where PI.ON=0 at vmentry or the PIR vector
>> > is mostly zero.  This saves at least 20 cycles (1%) per vmexit, as
>> > measured by kvm-unit-tests' inl_from_qemu test (20 runs):
>> > 
>> >               | enable_apicv=1  |  enable_apicv=0
>> >               | mean     stdev  |  mean     stdev
>> >     ----------|-----------------|------------------
>> >     before    | 5826     32.65  |  5765     47.09
>> >     after     | 5809     43.42  |  5777     77.02
>> > 
>> > Of course, any change in the right column is just placebo effect. :)
>> > The savings are bigger if interrupts are frequent.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> > ---
>> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> > @@ -521,6 +521,12 @@ static inline void pi_set_sn(struct pi_desc *pi_desc)
>> >  			(unsigned long *)&pi_desc->control);
>> >  }
>> >  
>> > +static inline void pi_clear_on(struct pi_desc *pi_desc)
>> > +{
>> > +	clear_bit(POSTED_INTR_ON,
>> > +  		  (unsigned long *)&pi_desc->control);
>> > +}
>> 
>> We should add an explicit smp_mb__after_atomic() for extra correctness,
>> because clear_bit() does not guarantee a memory barrier and we must make
>> sure that pir reads can't be reordered before it.
>> x86 clear_bit() currently uses locked instruction, though.
> 
> smp_mb__after_atomic is empty on x86 so it's
> a documentation thing, not a correctness thing anyway.

All atomics currently contain a barrier, but the code is also
future-proofing, not just documentation: implementation of clear_bit()
could drop the barrier and smp_mb__after_atomic() would then become a
real barrier.

Adding dma_mb__after_atomic() would be even better as this bug could
happen even on a uniprocessor with an assigned device, but people who
buy a SMP chip to run a UP kernel deserve it.