On 2017/7/31 20:31, Cornelia Huck wrote:
> On Mon, 31 Jul 2017 20:08:14 +0800
> "Longpeng (Mike)" <longpe...@huawei.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)
