KVM: fix rcu usage warning in kvm_arch_vcpu_ioctl_set_sregs()
PATCH v3 1/3 KVM: fix rcu usage warning in kvm_arch_vcpu_ioctl_set_sregs()
Fix:
[ 1001.499596] =================================================== [ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ] [ 1001.499601] --------------------------------------------------- [ 1001.499604] include/linux/kvm_host.h:301 invoked rcu_dereference_check() without protection! ...... [ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62 [ 1001.499638] Call Trace: [ 1001.499644] [] lockdep_rcu_dereference+0x9d/0xa5 [ 1001.499653] [] gfn_to_memslot+0x8d/0xc8 [kvm] [ 1001.499661] [] gfn_to_hva+0x16/0x3f [kvm] [ 1001.499669] [] kvm_read_guest_page+0x1e/0x5e [kvm] [ 1001.499681] [] kvm_read_guest_page_mmu+0x53/0x5e [kvm] [ 1001.499699] [] load_pdptrs+0x3f/0x9c [kvm] [ 1001.499705] [] ? vmx_set_cr0+0x507/0x517 [kvm_intel] [ 1001.499717] [] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm] [ 1001.499727] [] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]
--- arch/x86/kvm/x86.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +4 -1
@@ -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);
Signature · 1 line
1.7.3.4
PATCH v3 2/3 KVM: send IPI to vcpu only when it's in guest mode
We can interrupt the vcpu only when it's running in guest mode to reduce IPI
Also
1: No need atomically to read/write ->mode in vcpu's thread
2: reorganize struct kvm_vcpu to make ->mode and ->requests
in the same cache line explicitly
--- arch/ia64/kvm/kvm-ia64.c | 2 ++ arch/x86/kvm/x86.c | 16 ++++++++++------ include/linux/kvm_host.h | 22 +++++++++++++++++----- virt/kvm/kvm_main.c | 7 ++++++- 4 files changed, 35 insertions(+), 12 deletions(-)
arch/ia64/kvm/kvm-ia64.c +2 -0
@@ -662,6 +662,7 @@ again: goto vcpu_run_fail;srcu_read_unlock (&vcpu->kvm->srcu, idx); + vcpu->mode =IN_GUEST_MODE ;kvm_guest_enter ();/*
@@ -683,6 +684,7 @@ again:*/ barrier ();kvm_guest_exit (); + vcpu->mode =OUTSIDE_GUEST_MODE ;preempt_enable (); idx =srcu_read_lock (&vcpu->kvm->srcu);
arch/x86/kvm/x86.c +10 -6
@@ -5213,14 +5213,18 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)kvm_load_guest_fpu (vcpu);kvm_load_guest_xcr0 (vcpu); -atomic_set (&vcpu->guest_mode,1 ); -smp_wmb (); + vcpu->mode =IN_GUEST_MODE ; + +/* We should set ->mode before check ->requests, +* see the comment in make_all_cpus_request. +*/ +smp_mb ();local_irq_disable (); -if (!atomic_read (&vcpu->guest_mode) || vcpu->requests +if (vcpu->mode ==EXITING_GUEST_MODE || vcpu->requests ||need_resched () ||signal_pending (current)) { -atomic_set (&vcpu->guest_mode,0 ); + vcpu->mode =OUTSIDE_GUEST_MODE ;smp_wmb ();local_irq_enable ();preempt_enable ();
@@ -5256,7 +5260,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)kvm_get_msr (vcpu,MSR_IA32_TSC , &vcpu->arch.last_guest_tsc); -atomic_set (&vcpu->guest_mode,0 ); + vcpu->mode =OUTSIDE_GUEST_MODE ;smp_wmb ();local_irq_enable ();
@@ -6157,7 +6161,7 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) me =get_cpu ();if (cpu != me && (unsigned )cpu < nr_cpu_ids &&cpu_online (cpu)) -if (atomic_xchg (&vcpu->guest_mode,0 )) +if (kvm_vcpu_exiting_guest_mode (vcpu) ==IN_GUEST_MODE )smp_send_reschedule (cpu);put_cpu (); }
include/linux/kvm_host.h +17 -5
@@ -98,19 +98,26 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,int kvm_async_pf_wakeup_all (struct kvm_vcpu *vcpu);#endif +enum { +OUTSIDE_GUEST_MODE , +IN_GUEST_MODE , +EXITING_GUEST_MODE +}; +struct kvm_vcpu {struct kvm *kvm;#ifdef CONFIG_PREEMPT_NOTIFIERS struct preempt_notifier preempt_notifier;#endif +int cpu;int vcpu_id; -struct mutex mutex; -int cpu; -atomic_t guest_mode; -struct kvm_run *run; +int srcu_idx; +int mode;unsigned long requests;unsigned long guest_debug; -int srcu_idx; + +struct mutex mutex; +struct kvm_run *run;int fpu_active;int guest_fpu_loaded, guest_xcr0_loaded;
@@ -140,6 +147,11 @@ struct kvm_vcpu {struct kvm_vcpu_arch arch; }; +static inline int kvm_vcpu_exiting_guest_mode (struct kvm_vcpu *vcpu) +{ +return cmpxchg (&vcpu->mode,IN_GUEST_MODE ,EXITING_GUEST_MODE ); +} +/* * Some of the bitops functions do not support too long bitmaps. * This number must be determined not to exceed such limits.
virt/kvm/kvm_main.c +6 -1
@@ -153,7 +153,12 @@ 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) + +/* Set ->requests bit before we read ->mode */ +smp_mb (); + +if (cpus !=NULL && cpu !=-1 && cpu != me && +kvm_vcpu_exiting_guest_mode (vcpu) !=OUTSIDE_GUEST_MODE )cpumask_set_cpu (cpu, cpus); }if (unlikely (cpus ==NULL ))
Signature · 1 line
1.7.3.4
Re: [PATCH v3 2/3] KVM: send IPI to vcpu only when it's in guest mode
On Wed, Jan 12, 2011, Xiao Guangrong wrote about "[PATCH v3 2/3] KVM: send IPI to vcpu only when it's in guest mode":
Quoted 2 lines
We can interrupt the vcpu only when it's running in guest mode to reduce IPI
Hi,
I am afraid there's a risk of confusion between the new
vcpu->mode = IN_GUEST_MODE;
and the existing
is_guest_mode() (i.e., vcpu->arch.hflags & HF_GUEST_MASK)
The latter says that the virtual cpu is in guest mode (i.e., the guest used VMLAUNCH and is runnning a nested guest), while the former says that the physical CPU that this vcpu is currently being run on, is in guest mode - or in other words, this vcpu is currently running.
I'm not sure what is the best way to resolve this potential for confusion. Maybe on of them is better renamed (e.g., instead of vcpu->mode = IN_GUEST_MODE have something like vcpu->running = NOW_RUNNING). Or maybe some good comments need to to be written to explain the situation.
Actually, I just noticed that there's already a vcpu->guest_mode which you are apparently replacing, so the potential for confusion is already in the current code... I.e., in the current code, is_guest_mode(vcpu) does NOT check vcpu->guest_mode...
Signature · 4 lines
Nadav Har'El | Wednesday, Jan 12 2011, 7 Shevat 5771 [email protected] |----------------------------------------- Phone +972-523-790466, ICQ 13349191 |Hi! I'm a signature virus! Copy me into http://nadav.harel.org.il |your signature to help me spread!
Re: [PATCH v3 2/3] KVM: send IPI to vcpu only when it's in guest mode
Quoted 24 lines · Nadav Har'El
On Wed, Jan 12, 2011, Xiao Guangrong wrote about "[PATCH v3 2/3] KVM: send IPI to vcpu only when it's in guest mode":
Quoted 2 lines
We can interrupt the vcpu only when it's running in guest mode to reduce IPI
Hi,
I am afraid there's a risk of confusion between the new
vcpu->mode = IN_GUEST_MODE;and the existing
is_guest_mode() (i.e., vcpu->arch.hflags& HF_GUEST_MASK)The latter says that the virtual cpu is in guest mode (i.e., the guest used VMLAUNCH and is runnning a nested guest), while the former says that the physical CPU that this vcpu is currently being run on, is in guest mode - or in other words, this vcpu is currently running.
I'm not sure what is the best way to resolve this potential for confusion. Maybe on of them is better renamed (e.g., instead of
vcpu->mode=IN_GUEST_MODEhave something likevcpu->running=NOW_RUNNING). Or maybe some good comments need to to be written to explain the situation.
Quoted 5 lines
Actually, I just noticed that there's already a
vcpu->guest_modewhich you are apparently replacing, so the potential for confusion is already in the current code... I.e., in the current code,is_guest_mode(vcpu)does NOT checkvcpu->guest_mode...
Right. One is from the host's point of view, the other is from the guest's point of view. I'll rename is_guest_mode() to is_nested_guest().
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH v3 3/3 KVM: make make_all_cpus_request() lockless
In reply to Xiao GuangrongNow, we have 'vcpu->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
--- include/linux/kvm_host.h | 6 ------ virt/kvm/kvm_main.c | 9 +++------ 2 files changed, 3 insertions(+), 12 deletions(-)
include/linux/kvm_host.h +0 -6
@@ -224,7 +224,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;
@@ -731,11 +730,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)) {
virt/kvm/kvm_main.c +3 -6
@@ -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;/* Set ->requests bit before we read ->mode */
@@ -167,7 +165,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; }
@@ -433,7 +431,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);
Signature · 1 line
1.7.3.4
Quoted 19 lines · Xiao Guangrong
Fix:
[ 1001.499596] =================================================== [ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ] [ 1001.499601] --------------------------------------------------- [ 1001.499604] include/linux/kvm_host.h:301 invoked rcu_dereference_check() without protection! ...... [ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62 [ 1001.499638] Call Trace: [ 1001.499644] [] lockdep_rcu_dereference+0x9d/0xa5 [ 1001.499653] [] gfn_to_memslot+0x8d/0xc8 [kvm] [ 1001.499661] [] gfn_to_hva+0x16/0x3f [kvm] [ 1001.499669] [] kvm_read_guest_page+0x1e/0x5e [kvm] [ 1001.499681] [] kvm_read_guest_page_mmu+0x53/0x5e [kvm] [ 1001.499699] [] load_pdptrs+0x3f/0x9c [kvm] [ 1001.499705] [] ? vmx_set_cr0+0x507/0x517 [kvm_intel] [ 1001.499717] [] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm] [ 1001.499727] [] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]
Quoted 1 line
Signed-off-by Xiao Guangrong <[email protected]>
Ping ...?
Quoted 20 lines · Xiao Guangrong
Quoted 19 lines · Xiao Guangrong
Fix:
[ 1001.499596] =================================================== [ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ] [ 1001.499601] --------------------------------------------------- [ 1001.499604] include/linux/kvm_host.h:301 invoked rcu_dereference_check() without protection! ...... [ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62 [ 1001.499638] Call Trace: [ 1001.499644] [] lockdep_rcu_dereference+0x9d/0xa5 [ 1001.499653] [] gfn_to_memslot+0x8d/0xc8 [kvm] [ 1001.499661] [] gfn_to_hva+0x16/0x3f [kvm] [ 1001.499669] [] kvm_read_guest_page+0x1e/0x5e [kvm] [ 1001.499681] [] kvm_read_guest_page_mmu+0x53/0x5e [kvm] [ 1001.499699] [] load_pdptrs+0x3f/0x9c [kvm] [ 1001.499705] [] ? vmx_set_cr0+0x507/0x517 [kvm_intel] [ 1001.499717] [] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm] [ 1001.499727] [] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]
Quoted 3 lines
Quoted 1 line
Signed-off-by Xiao Guangrong <[email protected]>Ping ...?
Applied this fix. For the make_all_cpus_request optimization, can you show numbers with this new version? Because now there is LOCK# similarly to the spinlock.
Quoted 21 lines · Marcelo Tosatti
Quoted 20 lines · Xiao Guangrong
Quoted 19 lines · Xiao Guangrong
Fix:
[ 1001.499596] =================================================== [ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ] [ 1001.499601] --------------------------------------------------- [ 1001.499604] include/linux/kvm_host.h:301 invoked rcu_dereference_check() without protection! ...... [ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62 [ 1001.499638] Call Trace: [ 1001.499644] [] lockdep_rcu_dereference+0x9d/0xa5 [ 1001.499653] [] gfn_to_memslot+0x8d/0xc8 [kvm] [ 1001.499661] [] gfn_to_hva+0x16/0x3f [kvm] [ 1001.499669] [] kvm_read_guest_page+0x1e/0x5e [kvm] [ 1001.499681] [] kvm_read_guest_page_mmu+0x53/0x5e [kvm] [ 1001.499699] [] load_pdptrs+0x3f/0x9c [kvm] [ 1001.499705] [] ? vmx_set_cr0+0x507/0x517 [kvm_intel] [ 1001.499717] [] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm] [ 1001.499727] [] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Xiao Guangrong <[email protected]>Ping ...?
Applied this fix. For the
make_all_cpus_requestoptimization, can you show numbers with this new version? Because now there is LOCK# similarly to the spinlock.
Marcelo,
Sure :-), there is the simply test result of kernbench:
Before patch:
real 5m6.493s user 3m57.847s sys 9m7.115s
real 5m1.750s user 4m0.109s sys 9m10.192s
After patch:
real 5m0.140s user 3m57.956s sys 8m58.339s
real 4m56.314s user 4m0.303s sys 8m55.774s
Quoted 45 lines · Xiao Guangrong
Quoted 21 lines · Marcelo Tosatti
Quoted 20 lines · Xiao Guangrong
Quoted 19 lines · Xiao Guangrong
Fix:
[ 1001.499596] =================================================== [ 1001.499599] [ INFO: suspicious rcu_dereference_check() usage. ] [ 1001.499601] --------------------------------------------------- [ 1001.499604] include/linux/kvm_host.h:301 invoked rcu_dereference_check() without protection! ...... [ 1001.499636] Pid: 6035, comm: qemu-system-x86 Not tainted 2.6.37-rc6+ #62 [ 1001.499638] Call Trace: [ 1001.499644] [] lockdep_rcu_dereference+0x9d/0xa5 [ 1001.499653] [] gfn_to_memslot+0x8d/0xc8 [kvm] [ 1001.499661] [] gfn_to_hva+0x16/0x3f [kvm] [ 1001.499669] [] kvm_read_guest_page+0x1e/0x5e [kvm] [ 1001.499681] [] kvm_read_guest_page_mmu+0x53/0x5e [kvm] [ 1001.499699] [] load_pdptrs+0x3f/0x9c [kvm] [ 1001.499705] [] ? vmx_set_cr0+0x507/0x517 [kvm_intel] [ 1001.499717] [] kvm_arch_vcpu_ioctl_set_sregs+0x1f3/0x3c0 [kvm] [ 1001.499727] [] kvm_vcpu_ioctl+0x6a5/0xbc5 [kvm]Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Xiao Guangrong <[email protected]>Ping ...?
Applied this fix. For the
make_all_cpus_requestoptimization, can you show numbers with this new version? Because now there is LOCK# similarly to the spinlock.Marcelo,
Sure :-), there is the simply test result of kernbench:
Before patch:
real 5m6.493s user 3m57.847s sys 9m7.115sreal 5m1.750s user 4m0.109s sys 9m10.192s
Quoted 8 lines
After patch:
real 5m0.140s user 3m57.956s sys 8m58.339sreal 4m56.314s user 4m0.303s sys 8m55.774s
Nice. One disadvantageous side effect for the kvm_vcpu_kick path is that it can race with make_all_cpus_request, which is possibly doing unrelated, slower work (IPI'ing other vcpus, waiting for response).
Looks ok, but lets wait for more careful reviews before applying.
Quoted 22 lines · Marcelo Tosatti
Quoted 13 lines
Sure :-), there is the simply test result of kernbench:
Before patch:
real 5m6.493s user 3m57.847s sys 9m7.115sreal 5m1.750s user 4m0.109s sys 9m10.192sQuoted 8 lines
After patch:
real 5m0.140s user 3m57.956s sys 8m58.339sreal 4m56.314s user 4m0.303s sys 8m55.774s
Quoted 7 lines
Nice. One disadvantageous side effect for the
kvm_vcpu_kickpath is that it can race withmake_all_cpus_request, which is possibly doing unrelated, slower work (IPI'ing other vcpus, waiting for response).Looks ok, but lets wait for more careful reviews before applying.
Hi Avi, Marcelo,
I shall on vacation from 1.25 to 2.9, if you have comments on it i'll replay after coming back. :-)
Quoted 16 lines · Marcelo Tosatti
Quoted 16 lines
Before patch:
real 5m6.493s user 3m57.847s sys 9m7.115sreal 5m1.750s user 4m0.109s sys 9m10.192sAfter patch:
real 5m0.140s user 3m57.956s sys 8m58.339s
Quoted 8 lines
Quoted 3 lines
real 4m56.314s user 4m0.303s sys 8m55.774sNice. One disadvantageous side effect for the
kvm_vcpu_kickpath is that it can race withmake_all_cpus_request, which is possibly doing unrelated, slower work (IPI'ing other vcpus, waiting for response).
I think we're fine here. The kvm_vcpu_kick() check is
me = get_cpu();
if (cpu != me&& (unsigned)cpu< nr_cpu_ids&& cpu_online(cpu))
- if (atomic_xchg(&vcpu->guest_mode, 0))
+ if (kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE)
smp_send_reschedule(cpu);
put_cpu();
even if it did race, ->mode becomes EXITING_GUEST_MODE and we still avoid the IPI. Note that make_all_cpus_request() cleverly checks for != OUTSIDE_GUEST_MODE, so if it loses the race with kvm_vcpu_kick(), it still sends the IPI to be sure the vcpu loop sees vcpu->requests in time.
Quoted 1 line
Looks ok, but lets wait for more careful reviews before applying.
Patches applied.
Signature · 1 line
error compiling committee.c: too many arguments to function