linux-kernel - Re: [RFC] KVM: optimize the kvm_vcpu_on

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: <20170731143155.30a1b0dd@gondolin>
Date:   Mon, 31 Jul 2017 14:31:55 +0200
From:   Cornelia Huck <cohuck@...hat.com>
To:     "Longpeng (Mike)" <longpeng2@...wei.com>
Cc:     David Hildenbrand <david@...hat.com>, <pbonzini@...hat.com>,
        <rkrcmar@...hat.com>, <agraf@...e.com>, <borntraeger@...ibm.com>,
        <christoffer.dall@...aro.org>, <marc.zyngier@....com>,
        <james.hogan@...tec.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <weidong.huang@...wei.com>,
        <arei.gonglei@...wei.com>, <wangxinxin.wang@...wei.com>,
        <longpeng.mike@...il.com>
Subject: Re: [RFC] KVM: optimize the kvm_vcpu_on_spin

On Mon, 31 Jul 2017 20:08:14 +0800
"Longpeng (Mike)" <longpeng2@...wei.com> wrote:

> Hi David,
> 
> On 2017/7/31 19:31, David Hildenbrand wrote:

> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >> index 648b34c..f8f0d74 100644
> >> --- a/include/linux/kvm_host.h
> >> +++ b/include/linux/kvm_host.h
> >> @@ -272,6 +272,9 @@ struct kvm_vcpu {
> >>  	} spin_loop;
> >>  #endif
> >>  	bool preempted;
> >> +	/* If vcpu is in kernel-mode when preempted */
> >> +	bool in_kernmode;
> >> +  
> > 
> > Why do you have to store that ...
> >   
> 
> > [...]> +	me->in_kernmode = kvm_arch_vcpu_spin_kernmode(me);
> >>  	kvm_vcpu_set_in_spin_loop(me, true);
> >>  	/*
> >>  	 * We boost the priority of a VCPU that is runnable but not
> >> @@ -2351,6 +2353,8 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me)
> >>  				continue;
> >>  			if (swait_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu))
> >>  				continue;
> >> +			if (me->in_kernmode && !vcpu->in_kernmode)  
> > 
> > Wouldn't it be easier to simply have
> > 
> > in_kernel = kvm_arch_vcpu_in_kernel(me);
> > ...
> > if (in_kernel && !kvm_arch_vcpu_in_kernel(vcpu))
> > ...
> >   
> 
> I'm not sure whether the operation of get the vcpu's priority-level is
> expensive on all architectures, so I record it in kvm_sched_out() for
> minimal the extra cycles cost in kvm_vcpu_on_spin().

As it is now, this handling looks a bit inconsistent. You only update
the field on sched-out via preemption _or_ if kvm_vcpu_on_spin is
called for the vcpu. In most contexts, this field will have stale
content.

Also, would checking for kernel mode be more expensive than the various
other checks already done in this function?

[I like David's suggestion.]

> 
> >> +				continue;
> >>  			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
> >>  				continue;
> >>