[PATCH 1/3] KVM: fix rcu usage warning in kvm_arch_vcpu_ioctl_set_sregs()

[PATCH 1/3] KVM: fix rcu usage warning in kvm_arch_vcpu_ioctl_set_sregs()

[Xiao Guangrong]

Fix:
[ 1001.499596] ===================================================
[ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ]
[ 1001.499601] ---------------------------------------------------
	......
[ 1001.499633] stack backtrace:
[ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62
[ 1001.499638] Call Trace:
[ 1001.499644]  [<ffffffff81076cf6>] lockdep_rcu_dereference+0x9d/0xa5
[ 1001.499653]  [<ffffffffa02567a6>] gfn_to_memslot+0x8d/0xc8 [kvm]
[ 1001.499661]  [<ffffffffa0256824>] gfn_to_hva+0x16/0x3f [kvm]
[ 1001.499669]  [<ffffffffa02568f5>] kvm_read_guest_page+0x1e/0x5e [kvm]
[ 1001.499681]  [<ffffffffa026c24a>] kvm_read_guest_page_mmu+0x53/0x5e [kvm]
[ 1001.499699]  [<ffffffffa026c294>] load_pdptrs+0x3f/0x9c [kvm]
[ 1001.499705]  [<ffffffffa00d825e>] ? vmx_set_cr0+0x507/0x517 [kvm_intel]
[ 1001.499717]  [<ffffffffa026ca56>] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm]
[ 1001.499727]  [<ffffffffa025a410>] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 arch/x86/kvm/x86.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fa708c9..0b77e9c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5571,7 +5571,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
 	int mmu_reset_needed = 0;
-	int pending_vec, max_bits;
+	int pending_vec, max_bits, idx;
 	struct desc_ptr dt;
 
 	dt.size = sregs->idt.limit;
@@ -5600,10 +5600,13 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	kvm_x86_ops->set_cr4(vcpu, sregs->cr4);
 	if (sregs->cr4 & X86_CR4_OSXSAVE)
 		update_cpuid(vcpu);
+
+	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	if (!is_long_mode(vcpu) && is_pae(vcpu)) {
 		load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu));
 		mmu_reset_needed = 1;
 	}
+	srcu_read_unlock(&vcpu->kvm->srcu, idx);
 
 	if (mmu_reset_needed)
 		kvm_mmu_reset_context(vcpu);
-- 
1.7.3.4

[PATCH 2/3] KVM: send IPI to vcpu only when it's in guest mode

[Xiao Guangrong]

We can interrupt the vcpu only when it's running in guest mode
to reduce IPI.

It looks like only ia64 and x86 need to send IPI to other vcpus, so
i only add the implementation of 'vcpu->guest_mode' in ia64, but i
don't know ia64 well, please point out the right way for me if the
implementation is incorrect

And ia64 is not tested since i don't have ia64 box

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 arch/ia64/kvm/kvm-ia64.c |    2 ++
 virt/kvm/kvm_main.c      |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 70d224d..15c11b2 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -662,6 +662,7 @@ again:
 		goto vcpu_run_fail;
 
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
+	atomic_set(&vcpu->guest_mode, 1);
 	kvm_guest_enter();
 
 	/*
@@ -683,6 +684,7 @@ again:
 	 */
 	barrier();
 	kvm_guest_exit();
+	atomic_set(&vcpu->guest_mode, 0);
 	preempt_enable();
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b1b6cbb..6648c6e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -153,7 +153,8 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		if (kvm_make_check_request(req, vcpu))
 			continue;
 		cpu = vcpu->cpu;
-		if (cpus != NULL && cpu != -1 && cpu != me)
+		if (cpus != NULL && cpu != -1 && cpu != me &&
+		      atomic_read(&vcpu->guest_mode))
 			cpumask_set_cpu(cpu, cpus);
 	}
 	if (unlikely(cpus == NULL))
-- 
1.7.3.4

Re: [PATCH 2/3] KVM: send IPI to vcpu only when it's in guest mode

[Avi Kivity]

On 01/05/2011 11:38 AM, Xiao Guangrong wrote:
> We can interrupt the vcpu only when it's running in guest mode
> to reduce IPI.
>
> It looks like only ia64 and x86 need to send IPI to other vcpus, so
> i only add the implementation of 'vcpu->guest_mode' in ia64, but i
> don't know ia64 well, please point out the right way for me if the
> implementation is incorrect
>
> And ia64 is not tested since i don't have ia64 box
>
> Signed-off-by: Xiao Guangrong<xiaoguangrong@cn.fujitsu.com>
> ---
>   arch/ia64/kvm/kvm-ia64.c |    2 ++
>   virt/kvm/kvm_main.c      |    3 ++-
>   2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 70d224d..15c11b2 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -662,6 +662,7 @@ again:
>   		goto vcpu_run_fail;
>
>   	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +	atomic_set(&vcpu->guest_mode, 1);
>   	kvm_guest_enter();

I think it needs an smp_wmb() (technically x86 needs it too, but x86 is 
strongly ordered)

>
>   	/*
> @@ -683,6 +684,7 @@ again:
>   	 */
>   	barrier();
>   	kvm_guest_exit();
> +	atomic_set(&vcpu->guest_mode, 0);
>   	preempt_enable();
>
>   	idx = srcu_read_lock(&vcpu->kvm->srcu);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b1b6cbb..6648c6e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -153,7 +153,8 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
>   		if (kvm_make_check_request(req, vcpu))
>   			continue;
>   		cpu = vcpu->cpu;
> -		if (cpus != NULL&&  cpu != -1&&  cpu != me)
> +		if (cpus != NULL&&  cpu != -1&&  cpu != me&&
> +		      atomic_read(&vcpu->guest_mode))
>   			cpumask_set_cpu(cpu, cpus);

and smp_rmb() before the atomic_read().

>   	}
>   	if (unlikely(cpus == NULL))

Not sure if this is an optimization.  On one hand it removes an 
expensive IPI for the fraction of time the cpu is out of guest mode.  On 
the other hand it adds an unconditional cacheline bounce (and bounce back).

Hm. I see that ->guest_mode and ->requests are in fact in the same cache 
line.  So this is likely really an optimization.  We should probably 
reorganize kvm_vcpu so that this is made explicit.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] KVM: send IPI to vcpu only when it's in guest mode

[Xiao Guangrong]

On 01/05/2011 05:48 PM, Avi Kivity wrote:

>>       srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> +    atomic_set(&vcpu->guest_mode, 1);
>>       kvm_guest_enter();
> 
> I think it needs an smp_wmb() (technically x86 needs it too, but x86 is strongly ordered)

......

>> -        if (cpus != NULL&&  cpu != -1&&  cpu != me)
>> +        if (cpus != NULL&&  cpu != -1&&  cpu != me&&
>> +              atomic_read(&vcpu->guest_mode))
>>               cpumask_set_cpu(cpu, cpus);
> 
> and smp_rmb() before the atomic_read().

Yeah, you're right, i'll fix these in the next version.

> 
>>       }
>>       if (unlikely(cpus == NULL))
> 
> Not sure if this is an optimization.  On one hand it removes an expensive IPI for the fraction of time the cpu is out of guest mode.  On the other hand it adds an unconditional cacheline bounce (and bounce back).
> 
> Hm. I see that ->guest_mode and ->requests are in fact in the same cache line.  So this is likely really an optimization.  We should probably reorganize kvm_vcpu so that this is made explicit.
> 

OK, will do it in the separate patch in the future, thanks!

Re: [PATCH 2/3] KVM: send IPI to vcpu only when it's in guest mode

[Xiao Guangrong]

On 01/05/2011 05:48 PM, Avi Kivity wrote:
> On 01/05/2011 11:38 AM, Xiao Guangrong wrote:
>> We can interrupt the vcpu only when it's running in guest mode
>> to reduce IPI.
>>
>> It looks like only ia64 and x86 need to send IPI to other vcpus, so
>> i only add the implementation of 'vcpu->guest_mode' in ia64, but i
>> don't know ia64 well, please point out the right way for me if the
>> implementation is incorrect
>>
>> And ia64 is not tested since i don't have ia64 box
>>
>> Signed-off-by: Xiao Guangrong<xiaoguangrong@cn.fujitsu.com>
>> ---
>>   arch/ia64/kvm/kvm-ia64.c |    2 ++
>>   virt/kvm/kvm_main.c      |    3 ++-
>>   2 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>> index 70d224d..15c11b2 100644
>> --- a/arch/ia64/kvm/kvm-ia64.c
>> +++ b/arch/ia64/kvm/kvm-ia64.c
>> @@ -662,6 +662,7 @@ again:
>>           goto vcpu_run_fail;
>>
>>       srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> +    atomic_set(&vcpu->guest_mode, 1);
>>       kvm_guest_enter();
> 
> I think it needs an smp_wmb() (technically x86 needs it too, but x86 is strongly ordered)
> 

Hi Avi,

After read the code of ia64 more carefully, i noticed ia64 doesn't check 'vcpu->requests'
before go to guest mode, so it means we no need memory barrier here? right?

Certainly, we need an smb_mb between setting vcpu->mode and checking vcpu->request in x86.

[PATCH 3/3] KVM: make make_all_cpus_request() lockless

[Xiao Guangrong]

Now, we have 'vcpu->guest_mode' to judge whether need to send
ipi to other cpus, this way is very exact, so checking request
bit is needless, then we can drop the spinlock let it's collateral

There is the simple test result of kernbench:
  before patch:
   real    5m5.100s                                                                
   user    3m58.051s                                                               
   sys     9m4.742s                                      
                                                
   real    5m3.443s                                                                
   user    3m58.837s                                                               
   sys     9m6.007s

  after patch:
   real    5m0.769s                                                                
   user    3m59.666s                                                               
   sys     8m41.406s  

   real    4m57.980s                                                               
   user    3m59.302s                                                               
   sys     8m38.916s 

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 include/linux/kvm_host.h |    6 ------
 virt/kvm/kvm_main.c      |    9 +++------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b5021db..3e64133 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -212,7 +212,6 @@ struct kvm_memslots {
 
 struct kvm {
 	spinlock_t mmu_lock;
-	raw_spinlock_t requests_lock;
 	struct mutex slots_lock;
 	struct mm_struct *mm; /* userspace tied to this vm */
 	struct kvm_memslots *memslots;
@@ -719,11 +718,6 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 	set_bit(req, &vcpu->requests);
 }
 
-static inline bool kvm_make_check_request(int req, struct kvm_vcpu *vcpu)
-{
-	return test_and_set_bit(req, &vcpu->requests);
-}
-
 static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
 {
 	if (test_bit(req, &vcpu->requests)) {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6648c6e..34066ab 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -147,11 +147,9 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 
 	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
 
-	raw_spin_lock(&kvm->requests_lock);
-	me = smp_processor_id();
+	me = get_cpu();
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		if (kvm_make_check_request(req, vcpu))
-			continue;
+		kvm_make_request(req, vcpu);
 		cpu = vcpu->cpu;
 		if (cpus != NULL && cpu != -1 && cpu != me &&
 		      atomic_read(&vcpu->guest_mode))
@@ -163,7 +161,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		smp_call_function_many(cpus, ack_flush, NULL, 1);
 	else
 		called = false;
-	raw_spin_unlock(&kvm->requests_lock);
+	put_cpu();
 	free_cpumask_var(cpus);
 	return called;
 }
@@ -429,7 +427,6 @@ static struct kvm *kvm_create_vm(void)
 	kvm->mm = current->mm;
 	atomic_inc(&kvm->mm->mm_count);
 	spin_lock_init(&kvm->mmu_lock);
-	raw_spin_lock_init(&kvm->requests_lock);
 	kvm_eventfd_init(kvm);
 	mutex_init(&kvm->lock);
 	mutex_init(&kvm->irq_lock);
-- 
1.7.3.4

Re: [PATCH 3/3] KVM: make make_all_cpus_request() lockless

[Avi Kivity]

On 01/05/2011 11:40 AM, Xiao Guangrong wrote:
> Now, we have 'vcpu->guest_mode' to judge whether need to send
> ipi to other cpus, this way is very exact, so checking request
> bit is needless, then we can drop the spinlock let it's collateral

Clever.

> @@ -147,11 +147,9 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
>
>   	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
>
> -	raw_spin_lock(&kvm->requests_lock);
> -	me = smp_processor_id();
> +	me = get_cpu();
>   	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		if (kvm_make_check_request(req, vcpu))
> -			continue;
> +		kvm_make_request(req, vcpu);
>   		cpu = vcpu->cpu;
>   		if (cpus != NULL&&  cpu != -1&&  cpu != me&&
>   		atomic_read(&vcpu->guest_mode))
> @@ -163,7 +161,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
>   		smp_call_function_many(cpus, ack_flush, NULL, 1);
>   	else
>   		called = false;
> -	raw_spin_unlock(&kvm->requests_lock);
> +	put_cpu();
>   	free_cpumask_var(cpus);
>   	return called;
>   }

Maybe we can drop 'cpu != me' and then we don't need to disable preemption?

Can be done in a later patch.

Anyway, I really like this, requests_lock is an ugly wart.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/3] KVM: make make_all_cpus_request() lockless

[Xiao Guangrong]

On 01/05/2011 05:54 PM, Avi Kivity wrote:

>> +        kvm_make_request(req, vcpu);
>>           cpu = vcpu->cpu;
>>           if (cpus != NULL&&  cpu != -1&&  cpu != me&&
>>           atomic_read(&vcpu->guest_mode))
>> @@ -163,7 +161,7 @@ static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
>>           smp_call_function_many(cpus, ack_flush, NULL, 1);
>>       else
>>           called = false;
>> -    raw_spin_unlock(&kvm->requests_lock);
>> +    put_cpu();
>>       free_cpumask_var(cpus);
>>       return called;
>>   }
> 
> Maybe we can drop 'cpu != me' and then we don't need to disable preemption?
> 

Preemption should be disabled in the caller site of smp_call_function_many():

......

 * You must not call this function with disabled interrupts or from a
 * hardware interrupt handler or from a bottom half handler. Preemption
 * must be disabled when calling this function.
 */
void smp_call_function_many(const struct cpumask *mask,
			    smp_call_func_t func, void *info, bool wait)
{

......