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=kvm&m=136609583232031&w=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 kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
