2016-10-27 19:51+0300, Michael S. Tsirkin:
> On Thu, Oct 27, 2016 at 06:44:00PM +0200, Radim Krčmář wrote:
>> 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 <pbonz...@redhat.com>
>> >> > ---
>> >> > 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.
>
> Not doing dma so does not seem to make sense ...
IOMMU does -- it writes to the PIR and sets ON asynchronously.
> Why do you need a barrier on a UP kernel?
If pi_clear_on() doesn't contain a memory barrier (possible future),
then we have the following race: (pir[0] begins as 0.)
KVM | IOMMU
-------------------------------+-------------
pir_val = ACCESS_ONCE(pir[0]) |
| pir[0] = 123
| pi_set_on()
pi_clear_on() |
if (pir_val) |
ACCESS_ONCE() does not prevent the CPU to prefetch pir[0] (ACCESS_ONCE
does nothing in this patch), so if there was 0 in pir[0] before IOMMU
wrote to it, then our optimization to avoid the xchg would yield a false
negative and the interrupt would be lost.
