Re: [PATCH 1/3] KVM: add spinlock-exiting optimize framework

[Longpeng (Mike)]

On 2017/8/7 16:55, David Hildenbrand wrote:

> On 07.08.2017 10:44, Longpeng(Mike) wrote:
>> If the vcpu(me) exit due to request a usermode spinlock, then
>> the spinlock-holder may be preempted in usermode or kernmode.
>>
>> But if the vcpu(me) is in kernmode, then the holder must be
>> preempted in kernmode, so we should choose a vcpu in kernmode
>> as the most eligible candidate.
>>
>> For some architecture(e.g. arm/s390), spin/preempt_in_kernel()
>> are the same, but they are different for X86.
>>
>> Signed-off-by: Longpeng(Mike) <longpeng2@xxxxxxxxxx>
>> ---
>>  arch/mips/kvm/mips.c       | 10 ++++++++++
>>  arch/powerpc/kvm/powerpc.c | 10 ++++++++++
>>  arch/s390/kvm/kvm-s390.c   | 10 ++++++++++
>>  arch/x86/kvm/x86.c         | 10 ++++++++++
>>  include/linux/kvm_host.h   |  2 ++
>>  virt/kvm/arm/arm.c         | 10 ++++++++++
>>  virt/kvm/kvm_main.c        |  4 ++++
>>  7 files changed, 56 insertions(+)
>>
>> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
>> index d4b2ad1..e04e6b3 100644
>> --- a/arch/mips/kvm/mips.c
>> +++ b/arch/mips/kvm/mips.c
>> @@ -98,6 +98,16 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>>  	return !!(vcpu->arch.pending_exceptions);
>>  }
>>  
>> +bool kvm_arch_vcpu_spin_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>> +bool kvm_arch_vcpu_preempt_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>>  {
>>  	return 1;
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 1a75c0b..c573ddd 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -58,6 +58,16 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>>  	return !!(v->arch.pending_exceptions) || kvm_request_pending(v);
>>  }
>>  
>> +bool kvm_arch_vcpu_spin_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>> +bool kvm_arch_vcpu_preempt_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>>  {
>>  	return 1;
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index af09d34..f78cdc2 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2447,6 +2447,16 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>>  	return kvm_s390_vcpu_has_irq(vcpu, 0);
>>  }
>>  
>> +bool kvm_arch_vcpu_spin_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>> +bool kvm_arch_vcpu_preempt_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>>  void kvm_s390_vcpu_block(struct kvm_vcpu *vcpu)
>>  {
>>  	atomic_or(PROG_BLOCK_SIE, &vcpu->arch.sie_block->prog20);
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 6c97c82..04c6a1f 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8435,6 +8435,16 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>>  	return kvm_vcpu_running(vcpu) || kvm_vcpu_has_events(vcpu);
>>  }
>>  
>> +bool kvm_arch_vcpu_spin_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>> +bool kvm_arch_vcpu_preempt_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>>  {
>>  	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 890b706..9613620 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -798,6 +798,8 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  void kvm_arch_hardware_unsetup(void);
>>  void kvm_arch_check_processor_compat(void *rtn);
>>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
>> +bool kvm_arch_vcpu_spin_in_kernel(struct kvm_vcpu *vcpu);
>> +bool kvm_arch_vcpu_preempt_in_kernel(struct kvm_vcpu *vcpu);
>>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
>>  
>>  #ifndef __KVM_HAVE_ARCH_VM_ALLOC
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index a39a1e1..e45f780 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -416,6 +416,16 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>>  		&& !v->arch.power_off && !v->arch.pause);
>>  }
>>  
>> +bool kvm_arch_vcpu_spin_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
>> +bool kvm_arch_vcpu_preempt_in_kernel(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
> 
> Is the differentiation really necessary?
> 
> Can't you cache for x86 in all scenarios and simply introduce
> kvm_arch_vcpu_in_kernel() ?
> 


For X86 this is necessary,  I have no idea how to avoid this, hopes
someone could give me some suggestion. :)

> Otherwise, we have complexity that might just be avoided (e.g.
> kvm_arch_vcpu_spin_in_kernel must only be called on the loaded VCPU)
> 
>> +
>>  /* Just ensure a guest exit from a particular CPU */
>>  static void exit_vm_noop(void *info)
>>  {
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index f3f7427..0d0527b 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2324,12 +2324,14 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me)
>>  {
>>  	struct kvm *kvm = me->kvm;
>>  	struct kvm_vcpu *vcpu;
>> +	bool in_kern;
>>  	int last_boosted_vcpu = me->kvm->last_boosted_vcpu;
>>  	int yielded = 0;
>>  	int try = 3;
>>  	int pass;
>>  	int i;
>>  
>> +	in_kern = kvm_arch_vcpu_spin_in_kernel(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 (in_kern && !kvm_arch_vcpu_preempt_in_kernel(vcpu))
>> +				continue;
>>  			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
>>  				continue;
>>  
>>
> 
> 


-- 
Regards,
Longpeng(Mike)