Subject: Re: [PATCH 1/5] KVM: x86: avoid atomic operations on APICv vmentry
To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>
References: <1476469291-5039-1-git-send-email-pbonzini@redhat.com>
 <1476469291-5039-2-git-send-email-pbonzini@redhat.com>
 <20161026195344.GB4212@potion>
 <20161027003958-mutt-send-email-mst@kernel.org>
 <20161027164359.GE3452@potion>
 <20161027195030-mutt-send-email-mst@kernel.org>
 <20161027170611.GF3452@potion>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        yang.zhang.wz@gmail.com, feng.wu@intel.com
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <9e6946fd-8f55-26e5-de96-eb412475b6b4@redhat.com>
Date: Fri, 28 Oct 2016 11:39:44 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20161027170611.GF3452@potion>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3592
Lines: 88


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

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.

Preferences?

Thanks,

Paolo

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