Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On 04/21/2013 01:18 AM, Marcelo Tosatti wrote:
> On Thu, Apr 18, 2013 at 12:03:45PM +0800, Xiao Guangrong wrote:
>> On 04/18/2013 08:08 AM, Marcelo Tosatti wrote:
>>> On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote:
Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and
rename kvm_zap_all to kvm_free_all which is used to free all
memmory used by kvm mmu when vm is being destroyed, at this time,
no vcpu exists and mmu-notify has been unregistered, so we can
free the shadow pages out of mmu-lock
>>>
>>> Since there is no contention for mmu-lock its also not a problem to
>>> grab the lock right?
>>
>> This still has contention. Other mmu-notify can happen when we handle
>> ->release(). On the other handle, spin-lock is not preemptable.
>
> Don't think so:
Hi Marcelo,
The comment of ->release() says:
/*
* Called either by mmu_notifier_unregister or when the mm is
* being destroyed by exit_mmap, always before all pages are
* freed. This can run concurrently with other mmu notifier
* methods (the ones invoked outside the mm context)
>
> kvm_coalesced_mmio_free(kvm);
> #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
> mmu_notifier_unregister(>mmu_notifier, kvm->mm);
> #else
> kvm_arch_flush_shadow_all(kvm);
> #endif
> kvm_arch_destroy_vm(kvm);
The contention does not exist in the code you listed above. It happens when
vm abnormally exits (for example, VM is killed). Please refer to
commit 3ad3d90 (mm: mmu_notifier: fix freed page still mapped in secondary MMU).
The current mmu-notify code is wrong and i have posted a patch to fix it which
can be found at:
http://marc.info/?l=kvm=136609583232031=2
Maybe i misunderstand your meaning. This patch tries to use
kvm_mmu_invalid_all_pages
in ->release and rename kvm_zap_all to kvm_free_all. Do you mean we can still
use
mmu-lock in kvm_free_all()? If yes, I do not have strong opinion on this point
and
will keep kvm_free_all under the protection of mmu-lock.
>
>>> Automated verification of locking/srcu might complain.
>>
>> We hold slot-lock to free shadow page out of mmu-lock, it can avoid
>> the complain. No?
>
> Not if it realizes srcu is required to access the data structures.
It seems that kvm->srcu is only used to protect kvm->memslots, in kvm_memslots:
static inline struct kvm_memslots *kvm_memslots(struct kvm *kvm)
{
return rcu_dereference_check(kvm->memslots,
srcu_read_lock_held(>srcu)
|| lockdep_is_held(>slots_lock));
}
kvm->memslots can be safely accessed when hold kvm->srcu _or_ slots_lock.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On 04/21/2013 01:18 AM, Marcelo Tosatti wrote:
On Thu, Apr 18, 2013 at 12:03:45PM +0800, Xiao Guangrong wrote:
On 04/18/2013 08:08 AM, Marcelo Tosatti wrote:
On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote:
Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and
rename kvm_zap_all to kvm_free_all which is used to free all
memmory used by kvm mmu when vm is being destroyed, at this time,
no vcpu exists and mmu-notify has been unregistered, so we can
free the shadow pages out of mmu-lock
Since there is no contention for mmu-lock its also not a problem to
grab the lock right?
This still has contention. Other mmu-notify can happen when we handle
-release(). On the other handle, spin-lock is not preemptable.
Don't think so:
Hi Marcelo,
The comment of -release() says:
/*
* Called either by mmu_notifier_unregister or when the mm is
* being destroyed by exit_mmap, always before all pages are
* freed. This can run concurrently with other mmu notifier
* methods (the ones invoked outside the mm context)
kvm_coalesced_mmio_free(kvm);
#if defined(CONFIG_MMU_NOTIFIER) defined(KVM_ARCH_WANT_MMU_NOTIFIER)
mmu_notifier_unregister(kvm-mmu_notifier, kvm-mm);
#else
kvm_arch_flush_shadow_all(kvm);
#endif
kvm_arch_destroy_vm(kvm);
The contention does not exist in the code you listed above. It happens when
vm abnormally exits (for example, VM is killed). Please refer to
commit 3ad3d90 (mm: mmu_notifier: fix freed page still mapped in secondary MMU).
The current mmu-notify code is wrong and i have posted a patch to fix it which
can be found at:
http://marc.info/?l=kvmm=136609583232031w=2
Maybe i misunderstand your meaning. This patch tries to use
kvm_mmu_invalid_all_pages
in -release and rename kvm_zap_all to kvm_free_all. Do you mean we can still
use
mmu-lock in kvm_free_all()? If yes, I do not have strong opinion on this point
and
will keep kvm_free_all under the protection of mmu-lock.
Automated verification of locking/srcu might complain.
We hold slot-lock to free shadow page out of mmu-lock, it can avoid
the complain. No?
Not if it realizes srcu is required to access the data structures.
It seems that kvm-srcu is only used to protect kvm-memslots, in kvm_memslots:
static inline struct kvm_memslots *kvm_memslots(struct kvm *kvm)
{
return rcu_dereference_check(kvm-memslots,
srcu_read_lock_held(kvm-srcu)
|| lockdep_is_held(kvm-slots_lock));
}
kvm-memslots can be safely accessed when hold kvm-srcu _or_ slots_lock.
Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On Thu, Apr 18, 2013 at 12:03:45PM +0800, Xiao Guangrong wrote: > On 04/18/2013 08:08 AM, Marcelo Tosatti wrote: > > On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote: > >> Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and > >> rename kvm_zap_all to kvm_free_all which is used to free all > >> memmory used by kvm mmu when vm is being destroyed, at this time, > >> no vcpu exists and mmu-notify has been unregistered, so we can > >> free the shadow pages out of mmu-lock > > > > Since there is no contention for mmu-lock its also not a problem to > > grab the lock right? > > This still has contention. Other mmu-notify can happen when we handle > ->release(). On the other handle, spin-lock is not preemptable. Don't think so: kvm_coalesced_mmio_free(kvm); #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER) mmu_notifier_unregister(>mmu_notifier, kvm->mm); #else kvm_arch_flush_shadow_all(kvm); #endif kvm_arch_destroy_vm(kvm); > > Automated verification of locking/srcu might complain. > > We hold slot-lock to free shadow page out of mmu-lock, it can avoid > the complain. No? Not if it realizes srcu is required to access the data structures. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On Thu, Apr 18, 2013 at 12:03:45PM +0800, Xiao Guangrong wrote: On 04/18/2013 08:08 AM, Marcelo Tosatti wrote: On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote: Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and rename kvm_zap_all to kvm_free_all which is used to free all memmory used by kvm mmu when vm is being destroyed, at this time, no vcpu exists and mmu-notify has been unregistered, so we can free the shadow pages out of mmu-lock Since there is no contention for mmu-lock its also not a problem to grab the lock right? This still has contention. Other mmu-notify can happen when we handle -release(). On the other handle, spin-lock is not preemptable. Don't think so: kvm_coalesced_mmio_free(kvm); #if defined(CONFIG_MMU_NOTIFIER) defined(KVM_ARCH_WANT_MMU_NOTIFIER) mmu_notifier_unregister(kvm-mmu_notifier, kvm-mm); #else kvm_arch_flush_shadow_all(kvm); #endif kvm_arch_destroy_vm(kvm); Automated verification of locking/srcu might complain. We hold slot-lock to free shadow page out of mmu-lock, it can avoid the complain. No? Not if it realizes srcu is required to access the data structures. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On 04/18/2013 08:08 AM, Marcelo Tosatti wrote: > On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote: >> Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and >> rename kvm_zap_all to kvm_free_all which is used to free all >> memmory used by kvm mmu when vm is being destroyed, at this time, >> no vcpu exists and mmu-notify has been unregistered, so we can >> free the shadow pages out of mmu-lock > > Since there is no contention for mmu-lock its also not a problem to > grab the lock right? This still has contention. Other mmu-notify can happen when we handle ->release(). On the other handle, spin-lock is not preemptable. > > Automated verification of locking/srcu might complain. We hold slot-lock to free shadow page out of mmu-lock, it can avoid the complain. No? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote: > Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and > rename kvm_zap_all to kvm_free_all which is used to free all > memmory used by kvm mmu when vm is being destroyed, at this time, > no vcpu exists and mmu-notify has been unregistered, so we can > free the shadow pages out of mmu-lock Since there is no contention for mmu-lock its also not a problem to grab the lock right? Automated verification of locking/srcu might complain. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote: Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and rename kvm_zap_all to kvm_free_all which is used to free all memmory used by kvm mmu when vm is being destroyed, at this time, no vcpu exists and mmu-notify has been unregistered, so we can free the shadow pages out of mmu-lock Since there is no contention for mmu-lock its also not a problem to grab the lock right? Automated verification of locking/srcu might complain. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
On 04/18/2013 08:08 AM, Marcelo Tosatti wrote: On Tue, Apr 16, 2013 at 02:32:53PM +0800, Xiao Guangrong wrote: Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and rename kvm_zap_all to kvm_free_all which is used to free all memmory used by kvm mmu when vm is being destroyed, at this time, no vcpu exists and mmu-notify has been unregistered, so we can free the shadow pages out of mmu-lock Since there is no contention for mmu-lock its also not a problem to grab the lock right? This still has contention. Other mmu-notify can happen when we handle -release(). On the other handle, spin-lock is not preemptable. Automated verification of locking/srcu might complain. We hold slot-lock to free shadow page out of mmu-lock, it can avoid the complain. No? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and
rename kvm_zap_all to kvm_free_all which is used to free all
memmory used by kvm mmu when vm is being destroyed, at this time,
no vcpu exists and mmu-notify has been unregistered, so we can
free the shadow pages out of mmu-lock
Signed-off-by: Xiao Guangrong
---
arch/x86/include/asm/kvm_host.h |2 +-
arch/x86/kvm/mmu.c | 15 ++-
arch/x86/kvm/x86.c |9 -
3 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6f8ee18..a336055 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -771,7 +771,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int
slot);
void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
struct kvm_memory_slot *slot,
gfn_t gfn_offset, unsigned long mask);
-void kvm_mmu_zap_all(struct kvm *kvm);
+void kvm_mmu_free_all(struct kvm *kvm);
void kvm_arch_init_generation(struct kvm *kvm);
void kvm_mmu_invalid_mmio_sptes(struct kvm *kvm);
unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 12129b7..10c43ea 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4639,28 +4639,17 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
int slot)
spin_unlock(>mmu_lock);
}
-void kvm_mmu_zap_all(struct kvm *kvm)
+void kvm_mmu_free_all(struct kvm *kvm)
{
struct kvm_mmu_page *sp, *node;
LIST_HEAD(invalid_list);
- might_sleep();
-
- spin_lock(>mmu_lock);
restart:
- list_for_each_entry_safe(sp, node, >arch.active_mmu_pages, link) {
+ list_for_each_entry_safe(sp, node, >arch.active_mmu_pages, link)
if (kvm_mmu_prepare_zap_page(kvm, sp, _list))
goto restart;
- if (need_resched() || spin_needbreak(>mmu_lock)) {
- kvm_mmu_commit_zap_page(kvm, _list);
- cond_resched_lock(>mmu_lock);
- goto restart;
- }
- }
-
kvm_mmu_commit_zap_page(kvm, _list);
- spin_unlock(>mmu_lock);
}
static void kvm_mmu_zap_mmio_sptes(struct kvm *kvm)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d3dd0d5..4bb88f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6840,6 +6840,7 @@ void kvm_arch_sync_events(struct kvm *kvm)
void kvm_arch_destroy_vm(struct kvm *kvm)
{
+ kvm_mmu_free_all(kvm);
kvm_iommu_unmap_guest(kvm);
kfree(kvm->arch.vpic);
kfree(kvm->arch.vioapic);
@@ -7056,11 +7057,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
void kvm_arch_flush_shadow_all(struct kvm *kvm)
{
- int idx;
-
- idx = srcu_read_lock(>srcu);
- kvm_mmu_zap_all(kvm);
- srcu_read_unlock(>srcu, idx);
+ mutex_lock(>slots_lock);
+ kvm_mmu_invalid_memslot_pages(kvm, INVALID_ALL_SLOTS);
+ mutex_unlock(>slots_lock);
}
void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 15/15] KVM: MMU: replace kvm_zap_all with kvm_mmu_invalid_all_pages
Use kvm_mmu_invalid_all_pages in kvm_arch_flush_shadow_all and
rename kvm_zap_all to kvm_free_all which is used to free all
memmory used by kvm mmu when vm is being destroyed, at this time,
no vcpu exists and mmu-notify has been unregistered, so we can
free the shadow pages out of mmu-lock
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
---
arch/x86/include/asm/kvm_host.h |2 +-
arch/x86/kvm/mmu.c | 15 ++-
arch/x86/kvm/x86.c |9 -
3 files changed, 7 insertions(+), 19 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6f8ee18..a336055 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -771,7 +771,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int
slot);
void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
struct kvm_memory_slot *slot,
gfn_t gfn_offset, unsigned long mask);
-void kvm_mmu_zap_all(struct kvm *kvm);
+void kvm_mmu_free_all(struct kvm *kvm);
void kvm_arch_init_generation(struct kvm *kvm);
void kvm_mmu_invalid_mmio_sptes(struct kvm *kvm);
unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 12129b7..10c43ea 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4639,28 +4639,17 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
int slot)
spin_unlock(kvm-mmu_lock);
}
-void kvm_mmu_zap_all(struct kvm *kvm)
+void kvm_mmu_free_all(struct kvm *kvm)
{
struct kvm_mmu_page *sp, *node;
LIST_HEAD(invalid_list);
- might_sleep();
-
- spin_lock(kvm-mmu_lock);
restart:
- list_for_each_entry_safe(sp, node, kvm-arch.active_mmu_pages, link) {
+ list_for_each_entry_safe(sp, node, kvm-arch.active_mmu_pages, link)
if (kvm_mmu_prepare_zap_page(kvm, sp, invalid_list))
goto restart;
- if (need_resched() || spin_needbreak(kvm-mmu_lock)) {
- kvm_mmu_commit_zap_page(kvm, invalid_list);
- cond_resched_lock(kvm-mmu_lock);
- goto restart;
- }
- }
-
kvm_mmu_commit_zap_page(kvm, invalid_list);
- spin_unlock(kvm-mmu_lock);
}
static void kvm_mmu_zap_mmio_sptes(struct kvm *kvm)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d3dd0d5..4bb88f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6840,6 +6840,7 @@ void kvm_arch_sync_events(struct kvm *kvm)
void kvm_arch_destroy_vm(struct kvm *kvm)
{
+ kvm_mmu_free_all(kvm);
kvm_iommu_unmap_guest(kvm);
kfree(kvm-arch.vpic);
kfree(kvm-arch.vioapic);
@@ -7056,11 +7057,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
void kvm_arch_flush_shadow_all(struct kvm *kvm)
{
- int idx;
-
- idx = srcu_read_lock(kvm-srcu);
- kvm_mmu_zap_all(kvm);
- srcu_read_unlock(kvm-srcu, idx);
+ mutex_lock(kvm-slots_lock);
+ kvm_mmu_invalid_memslot_pages(kvm, INVALID_ALL_SLOTS);
+ mutex_unlock(kvm-slots_lock);
}
void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
--
1.7.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
