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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161027003958-mutt-send-email-mst@kernel.org>
Date:   Thu, 27 Oct 2016 00:42:45 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Radim Krčmář <rkrcmar@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, yang.zhang.wz@...il.com, feng.wu@...el.com
Subject: Re: [PATCH 1/5] KVM: x86: avoid atomic operations on APICv vmentry

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@...hat.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);
>    ^^
>    bad whitespace.
> 
> > +}
> 
> 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.

> > +
> >  static inline int pi_test_on(struct pi_desc *pi_desc)
> >  {
> >  	return test_bit(POSTED_INTR_ON,
> 
> Other than that,
> 
> Reviewed-by: Radim Krčmář <rkrcmar@...hat.com>