do not free active mmu pages in free_mmu_pages()
PATCH 1/1 do not free active mmu pages in free_mmu_pages()
free_mmu_pages() should only undo what alloc_mmu_pages() does.
arch/x86/kvm/mmu.c +0 -8
@@ -2638,14 +2638,6 @@ EXPORT_SYMBOL_GPL(kvm_disable_tdp);static void free_mmu_pages (struct kvm_vcpu *vcpu) { -struct kvm_mmu_page *sp; - -while (!list_empty (&vcpu->kvm->arch.active_mmu_pages)) { - sp =container_of (vcpu->kvm->arch.active_mmu_pages.next, - struct kvm_mmu_page, link); -kvm_mmu_zap_page (vcpu->kvm, sp); -cond_resched (); - }free_page ((unsigned long )vcpu->arch.mmu.pae_root); }
--
Gleb.
Quoted 13 lines · Gleb Natapov
free_mmu_pages()should only undo whatalloc_mmu_pages()does.Signed-off-by Gleb Natapov <[email protected]>arch/x86/kvm/mmu.c +0
-1@@ -2638,14 +2638,6 @@ EXPORT_SYMBOL_GPL(kvm_disable_tdp);static void free_mmu_pages (struct kvm_vcpu *vcpu) { -struct kvm_mmu_page *sp;
arch/x86/kvm/mmu.c EXPORT_SYMBOL_GPL(kvm_disable_tdp); · line 2643
Quoted 10 lines
arch/x86/kvm/mmu.c +0
-7- -while (!list_empty (&vcpu->kvm->arch.active_mmu_pages)) { - sp =container_of (vcpu->kvm->arch.active_mmu_pages.next, - struct kvm_mmu_page, link); -kvm_mmu_zap_page (vcpu->kvm, sp); -cond_resched (); - }free_page ((unsigned long )vcpu->arch.mmu.pae_root); }
I think this is correct, but the patch leaves the function name wrong. Rename, or perhaps just open code into callers?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Gleb Natapov
free_mmu_pages()should only undo whatalloc_mmu_pages()does.Signed-off-by Gleb Natapov <[email protected]>arch/x86/kvm/mmu.c +0
-0@@ -2638,14 +2638,6 @@ EXPORT_SYMBOL_GPL(kvm_disable_tdp);static void free_mmu_pages (struct kvm_vcpu *vcpu) {
arch/x86/kvm/mmu.c EXPORT_SYMBOL_GPL(kvm_disable_tdp); · line 2642
Quoted 10 lines
arch/x86/kvm/mmu.c +0
-8-struct kvm_mmu_page *sp; - -while (!list_empty (&vcpu->kvm->arch.active_mmu_pages)) { - sp =container_of (vcpu->kvm->arch.active_mmu_pages.next, - struct kvm_mmu_page, link); -kvm_mmu_zap_page (vcpu->kvm, sp); -cond_resched (); - }free_page ((unsigned long )vcpu->arch.mmu.pae_root); }
Doesnt the vm shutdown path rely on the while loop you removed to free all shadow pages before freeing the mmu kmem caches, if mmu notifiers is disabled?
And how harmful is that loop? Zaps the entire cache on cpu hotunplug?
Quoted 10 lines · Marcelo Tosatti
Quoted 9 lines · Gleb Natapov
free_mmu_pages()should only undo whatalloc_mmu_pages()does.Signed-off-by Gleb Natapov <[email protected]>arch/x86/kvm/mmu.c +0
-0@@ -2638,14 +2638,6 @@ EXPORT_SYMBOL_GPL(kvm_disable_tdp);
Quoted 17 lines
arch/x86/kvm/mmu.c EXPORT_SYMBOL_GPL(kvm_disable_tdp); · line 2642
Quoted 12 lines
arch/x86/kvm/mmu.c +0
-8static void free_mmu_pages (struct kvm_vcpu *vcpu) { -struct kvm_mmu_page *sp; - -while (!list_empty (&vcpu->kvm->arch.active_mmu_pages)) { - sp =container_of (vcpu->kvm->arch.active_mmu_pages.next, - struct kvm_mmu_page, link); -kvm_mmu_zap_page (vcpu->kvm, sp); -cond_resched (); - }free_page ((unsigned long )vcpu->arch.mmu.pae_root); }Doesnt the vm shutdown path rely on the while loop you removed to free all shadow pages before freeing the mmu kmem caches, if mmu notifiers is disabled?
Shouldn't mmu_free_roots() on all vcpus clear all mmu pages?
Quoted 2 lines
And how harmful is that loop? Zaps the entire cache on cpu hotunplug?
KVM doesn't support vcpu destruction, but destruction is called anyway on various error conditions. The one that easy to trigger is to create vcpu with the same id simultaneously from two threads. The result is OOPs in random places.
--
Gleb.
Quoted 5 lines · Gleb Natapov
Quoted 4 lines
Doesnt the vm shutdown path rely on the while loop you removed to free all shadow pages before freeing the mmu kmem caches, if mmu notifiers is disabled?
Shouldn't
mmu_free_roots()on all vcpus clear all mmu pages?
No. It only zaps the present root on every vcpu, but not the children.
Quoted 6 lines
Quoted 2 lines
And how harmful is that loop? Zaps the entire cache on cpu hotunplug?
KVM doesn't support vcpu destruction, but destruction is called anyway on various error conditions. The one that easy to trigger is to create vcpu with the same id simultaneously from two threads. The result is OOPs in random places.
mmu_lock should be held there, and apparently it is not.
Quoted 10 lines · Marcelo Tosatti
Quoted 5 lines · Gleb Natapov
Quoted 4 lines
Doesnt the vm shutdown path rely on the while loop you removed to free all shadow pages before freeing the mmu kmem caches, if mmu notifiers is disabled?
Shouldn't
mmu_free_roots()on all vcpus clear all mmu pages?No. It only zaps the present root on every vcpu, but not the children.
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
And how harmful is that loop? Zaps the entire cache on cpu hotunplug?
KVM doesn't support vcpu destruction, but destruction is called anyway on various error conditions. The one that easy to trigger is to create vcpu with the same id simultaneously from two threads. The result is OOPs in random places.
mmu_lockshould be held there, and apparently it is not.
Yeah, my first solution was to add mmu_lock, but why function that gets vcpu as an input should destroy data structure that is global for the VM. There is kvm_mmu_zap_all() that does same thing (well almost) and also does proper locking. Shouldn't it be called during VM destruction instead?
--
Gleb.
Quoted 4 lines · Gleb Natapov
Quoted 2 lines
mmu_lockshould be held there, and apparently it is not.Yeah, my first solution was to add
mmu_lock, but why function that gets vcpu as an input should destroy data structure that is global for the VM.
Point.
Quoted 2 lines
There is
kvm_mmu_zap_all()that does same thing (well almost) and also does proper locking. Shouldn't it be called during VM destruction instead?
Yes, that would better (which happens implicitly with mmu notifiers ->release).
Quoted 6 lines · Marcelo Tosatti
Quoted 4 lines · Gleb Natapov
Quoted 2 lines
mmu_lockshould be held there, and apparently it is not.Yeah, my first solution was to add
mmu_lock, but why function that gets vcpu as an input should destroy data structure that is global for the VM.
Quoted 7 lines
Point.
Quoted 2 lines
There is
kvm_mmu_zap_all()that does same thing (well almost) and also does proper locking. Shouldn't it be called during VM destruction instead?Yes, that would better (which happens implicitly with mmu notifiers ->release).
OK. I'll send new patch.
--
Gleb.