Date: Thu, 27 Oct 2016 19:51:33 +0300
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, yang.zhang.wz@gmail.com, feng.wu@intel.com
Subject: Re: [PATCH 1/5] KVM: x86: avoid atomic operations on APICv vmentry
Message-ID: <20161027195030-mutt-send-email-mst@kernel.org>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161027164359.GE3452@potion>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2504
Lines: 57

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 ...
Why do you need a barrier on a UP kernel?

-- 
MST