From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrea Arcangeli Subject: Re: [PATCH] kvm memslot read-locking with mmu_lock Date: Tue, 22 Jan 2008 15:32:10 +0100 Message-ID: <20080122143210.GC7331@v2.random> References: <20080121123710.GF6970@v2.random> <4795F3F0.90403@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Avi Kivity Return-path: Content-Disposition: inline In-Reply-To: <4795F3F0.90403-atKUWr5tajBWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org On Tue, Jan 22, 2008 at 03:47:28PM +0200, Avi Kivity wrote: > Andrea Arcangeli wrote: >> This adds locking to the memslots so they can be looked up with only >> the mmu_lock. Entries with memslot->userspace_addr have to be ignored >> because they're not fully inserted yet. >> >> > What is the motivation for this? Calls from mmu notifiers that don't have > mmap_sem held? Exactly. > >> /* Allocate page dirty bitmap if needed */ >> @@ -311,14 +320,18 @@ int __kvm_set_memory_region(struct kvm *kvm, >> memset(new.dirty_bitmap, 0, dirty_bytes); >> } >> + spin_lock(&kvm->mmu_lock); >> if (mem->slot >= kvm->nmemslots) >> kvm->nmemslots = mem->slot + 1; >> *memslot = new; >> + spin_unlock(&kvm->mmu_lock); >> r = kvm_arch_set_memory_region(kvm, mem, old, user_alloc); >> if (r) { >> + spin_lock(&kvm->mmu_lock); >> *memslot = old; >> + spin_unlock(&kvm->mmu_lock); >> goto out_free; >> } >> > > This is arch independent code, I'm surprised mmu_lock is visible here? The mmu_lock is arch independent as far as I can tell. Pretty much like the mm->page_table_lock is also independent. All archs will have some form of shadow pagetables in software or hardware, and mmu_lock is the lock to take to serialize the pagetable updates and it also allows to walk the memslots in readonly mode. > What are the new lookup rules? We don't hold mmu_lock everywhere we look > up a gfn, do we? It's safe to loop over the memslots by just skipping the ones with userland_addr == 0 by only holding the mmu_lock. The memslots contents can't change by holding the mmu_lock. The mmu_lock also serializes the rmap structures inside the memslot and the spte updates. So by just taking the mmu_lock it's trivial to do "search memslot", "translate the hva to its relative rmapp", "find all sptes relative to the hva and overwrite them with nonpresent-fault". If the mmu_notifiers would have been registered inside the vma things would look very different in this area and it might have been possible to embed the mmu-notifier inside the memslot itself, to avoid the "search memslot" op. ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/