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

[Michael S. Tsirkin]

On Fri, Oct 28, 2016 at 11:39:44AM +0200, Paolo Bonzini wrote:
> 
> 
> On 27/10/2016 19:06, Radim KrÄmÃÅ wrote:
> > 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 <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.
> >>
> >> Not doing dma so does not seem to make sense ...
> > 
> > IOMMU does -- it writes to the PIR and sets ON asynchronously.
> 
> I can use either __smp_mb__after_atomic or virt_mb__after_atomic.  The
> difference is documentation only, since all of them are
> compiler-barriers only on x86.

A comment is also an option.

> Preferences?
> 
> Thanks,
> 
> Paolo

virt_ is for a VM guest. Pls don't use for host side code.
I thought it's clear enough but maybe I should add
more documentation.

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