kvm memslot read-locking with mmu_lock

PATCH In review Andrea Arcangeli 3 participants 6 messages Jan 21, 2008 – Jan 23, 2008 5 messages not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Andrea Arcangeli

author

2008-01-21 12:37 UTC #0 lore raw

PATCH 1/1 kvm memslot read-locking with mmu_lock

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.

Signed-off-by Andrea Arcangeli <andrea-atKUWr5tajBWk0Htik3J/[email protected]>

arch/x86/kvm/x86.c +14 -8

index 8a90403..35a2ee0 100644
@@ -3219,14 +3249,20 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
 	 */
 	if (!user_alloc) {
 		if (npages && !old.rmap) {
-			memslot->userspace_addr = do_mmap(NULL, 0,
-						     npages * PAGE_SIZE,
-						     PROT_READ | PROT_WRITE,
-						     MAP_SHARED | MAP_ANONYMOUS,
-						     0);
-
-			if (IS_ERR((void *)memslot->userspace_addr))
-				return PTR_ERR((void *)memslot->userspace_addr);
+			unsigned long userspace_addr;
+			
+			userspace_addr = do_mmap(NULL, 0,
+						 npages * PAGE_SIZE,
+						 PROT_READ | PROT_WRITE,
+						 MAP_SHARED | MAP_ANONYMOUS,
+						 0);
+			if (IS_ERR((void *)userspace_addr))
+				return PTR_ERR((void *)userspace_addr);
+
+			/* set userspace_addr atomically for kvm_hva_to_rmapp */
+			spin_lock(&kvm->mmu_lock);
+			memslot->userspace_addr = userspace_addr;
+			spin_unlock(&kvm->mmu_lock);
 		} else {
 			if (!old.user_alloc && old.rmap) {
 				int ret;

virt/kvm/kvm_main.c +13 -1

index 4295623..a67e38f 100644
@@ -298,7 +299,15 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		memset(new.rmap, 0, npages * sizeof(*new.rmap));
 
 		new.user_alloc = user_alloc;
-		new.userspace_addr = mem->userspace_addr;
+		/*
+		 * hva_to_rmmap() serialzies with the mmu_lock and to be
+		 * safe it has to ignore memslots with !user_alloc &&
+		 * !userspace_addr.
+		 */
+		if (user_alloc)
+			new.userspace_addr = mem->userspace_addr;
+		else
+			new.userspace_addr = 0;
 	}
 
 	/* 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 SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2008-01-22 13:47 UTC #2 lore raw

Quoted 5 lines · Andrea Arcangeli

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?

Quoted 12 lines

virt/kvm/kvm_main.c +1 -0

 	/* 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;

virt/kvm/kvm_main.c int __kvm_set_memory_region(struct kvm *kvm, · line 337

Quoted 10 lines

virt/kvm/kvm_main.c +3 -0

+	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?

What are the new lookup rules? We don't hold mmu_lock everywhere we look up a gfn, do we?

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
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/

A message that is not in the archive (sent off-list or not yet received): 4795F3F0.90403-atKUWr5tajBWk0Htik3J/[email protected]

Andrea Arcangeli

author

2008-01-22 14:32 UTC #4 lore raw

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Andrea Arcangeli
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.

Quoted 12 lines

/* Allocate page dirty bitmap if needed */

virt/kvm/kvm_main.c +1 -0

@@ -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) {

Quoted 8 lines

virt/kvm/kvm_main.c int __kvm_set_memory_region(struct kvm *kvm, · line 337
Quoted 6 lines
virt/kvm/kvm_main.c +2 -0
+		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.

Quoted 2 lines

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2008-01-22 14:38 UTC #6 lore raw

Quoted 11 lines · Andrea Arcangeli

Quoted 3 lines
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.

Well, s390 has everything in hardware, but I suppose they can just ignore the lock.

Quoted 4 lines

Quoted 3 lines
What are the new lookup rules? We don't hold mmu_lock everywhere we look up a gfn, do we?

Quoted 9 lines

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".

But we lookup memslots in parallel in the guest walker and similar places, relying on mmap_sem being taken for read.

Maybe we need a rwlock instead, and drop this overloaded usage of mmap_sem.

Quoted 5 lines

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.

Nothing guarantees a 1:1 mapping between memslots and vma. You can have a vma backing multiple memslots, or a memslot spanning multiple vmas.

Signature · 7 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
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/

A message that is not in the archive (sent off-list or not yet received): 4795FFF9.8010400-atKUWr5tajBWk0Htik3J/[email protected]

Andrea Arcangeli

author

2008-01-22 14:50 UTC #8 lore raw

Quoted 13 lines · Avi Kivity

Quoted 11 lines · Andrea Arcangeli
Quoted 3 lines
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.

Quoted 2 lines

Well, s390 has everything in hardware, but I suppose they can just ignore the lock.

If they don't take it in their lowlevel it's enough I think. It'll still be useful to lookup memslots like in every other arch.

Quoted 2 lines

But we lookup memslots in parallel in the guest walker and similar places, relying on mmap_sem being taken for read.

That's ok. The vmas could also be looked up either with mmap_sam in read mode or with only the page_table_lock taken (these days it doesn't work anymore with ptlocks). The mmu_lock is only taken in the modifications of the memslots, the lookups don't require it. The modifications have to take both mmap_sem in write mode and the mmu_lock. Lookups requires either mmap_sem in read mode or the mmu_lock. The lookups with mmap_sem in read mode won't ever see a userspace_addr =0, the mmu_lock have to skip over the userspace_addr=0 instead.

Quoted 1 line

Maybe we need a rwlock instead, and drop this overloaded usage of mmap_sem.

Adding another lock just for the memslots should be feasible. However it can't be a semaphore or it won't work for the mmu-notifiers. A rwlock would be ok, but then all memslots lookups will have to be updated to check for userspace_addr != 0 like the kvm_hva_to_rmapp does.

Quoted 2 lines

Nothing guarantees a 1:1 mapping between memslots and vma. You can have a vma backing multiple memslots, or a memslot spanning multiple vmas.

Yes, that's why it would add complications to have it in the vma, one would need to add a vma split to guarantee the 1:1 mapping, and then it'd be a bit faster too (though the memslots can easily be rbtree-indexed).

A message that is not in the archive (sent off-list or not yet received): [email protected]

Carsten Otte

2008-01-23 08:15 UTC #10 lore raw

Quoted 11 lines · Andrea Arcangeli

Quoted 10 lines · Avi Kivity
Quoted 9 lines · Andrea Arcangeli
Quoted 2 lines
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.

Quoted 5 lines

Quoted 2 lines
Well, s390 has everything in hardware, but I suppose they can just ignore the lock.
If they don't take it in their lowlevel it's enough I think. It'll still be useful to lookup memslots like in every other arch.

The lock won't be a performance issue for s390. We don't need to look them up frequently afaics.