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]

Date:   Mon, 31 Jul 2017 21:01:59 +0800
From:   "Longpeng (Mike)" <longpeng2@...wei.com>
To:     Cornelia Huck <cohuck@...hat.com>,
        David Hildenbrand <david@...hat.com>
CC:     <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 2017/7/31 20:31, Cornelia Huck wrote:

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


Hi Cornelia & David,

I'll take your suggestion, thanks :)

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


-- 
Regards,
Longpeng(Mike)