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 <[email protected]> >> > --- >> > 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.
