[PATCH 1/2] KVM: introducing wrapper function for creating/destroying dirty bitmaps

[PATCH 1/2] KVM: introducing wrapper function for creating/destroying dirty bitmaps

[Takuya Yoshikawa]

This makes it easy to change the way of allocating/freeing dirty bitmaps.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---
 virt/kvm/kvm_main.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4a71faa..e4f5762 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -439,6 +439,15 @@ out_err_nodisable:
 	return ERR_PTR(r);
 }
 
+static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
+{
+	if (!memslot->dirty_bitmap)
+		return;
+
+	vfree(memslot->dirty_bitmap);
+	memslot->dirty_bitmap = NULL;
+}
+
 /*
  * Free any memory in @free but not in @dont.
  */
@@ -451,7 +460,7 @@ static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
 		vfree(free->rmap);
 
 	if (!dont || free->dirty_bitmap != dont->dirty_bitmap)
-		vfree(free->dirty_bitmap);
+		kvm_destroy_dirty_bitmap(free);
 
 
 	for (i = 0; i < KVM_NR_PAGE_SIZES - 1; ++i) {
@@ -462,7 +471,6 @@ static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
 	}
 
 	free->npages = 0;
-	free->dirty_bitmap = NULL;
 	free->rmap = NULL;
 }
 
@@ -524,6 +532,18 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot)
+{
+	unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(memslot);
+
+	memslot->dirty_bitmap = vmalloc(dirty_bytes);
+	if (!memslot->dirty_bitmap)
+		return -ENOMEM;
+
+	memset(memslot->dirty_bitmap, 0, dirty_bytes);
+	return 0;
+}
+
 /*
  * Allocate some memory and give it an address in the guest physical address
  * space.
@@ -657,12 +677,8 @@ skip_lpage:
 
 	/* Allocate page dirty bitmap if needed */
 	if ((new.flags & KVM_MEM_LOG_DIRTY_PAGES) && !new.dirty_bitmap) {
-		unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(&new);
-
-		new.dirty_bitmap = vmalloc(dirty_bytes);
-		if (!new.dirty_bitmap)
+		if (kvm_create_dirty_bitmap(&new) < 0)
 			goto out_free;
-		memset(new.dirty_bitmap, 0, dirty_bytes);
 		/* destroy any largepage mappings for dirty tracking */
 		if (old.npages)
 			flush_shadow = 1;
-- 
1.7.0.4

[PATCH 2/2] KVM: x86: pre-allocate one more dirty bitmap to avoid vmalloc() in kvm_vm_ioctl_get_dirty_log()

[Takuya Yoshikawa]

Currently x86's kvm_vm_ioctl_get_dirty_log() needs to allocate a bitmap by
vmalloc() which will be used in the next logging and this has been causing
bad effects to VGA and live-migration: vmalloc() consumes extra systime,
triggers tlb flush, etc.

This patch resolves this issue by pre-allocating one more bitmap and switching
between the two bitmaps during dirty logging.

Performance improvement:
  In the usual Desktop environment, kvm_vm_ioctl_get_dirty_log() get about
  twice faster than the original one for graphical updates.

  During live-migration with low work load, the improvement ratio was
  about three when guest memory was 4GB.

Note:
  Though this patch introduces some ifdefs, we tried not to mixing these
  with other parts to keep the code as clean as possible.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---
 arch/x86/include/asm/kvm_host.h |    2 ++
 arch/x86/kvm/x86.c              |   16 +++++-----------
 include/linux/kvm_host.h        |    3 +++
 virt/kvm/kvm_main.c             |   12 ++++++++++--
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0cd0f29..c516f1c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -81,6 +81,8 @@
 #define KVM_NR_FIXED_MTRR_REGION 88
 #define KVM_NR_VAR_MTRR 8
 
+#define __KVM_DIRTY_LOG_DOUBLE_BUFFERING
+
 extern spinlock_t kvm_lock;
 extern struct list_head vm_list;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5e5cd8d..0871ef8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2804,18 +2804,15 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		kvm_mmu_slot_remove_write_access(kvm, log->slot);
 		spin_unlock(&kvm->mmu_lock);
 
-		r = -ENOMEM;
-		dirty_bitmap = vmalloc(n);
-		if (!dirty_bitmap)
-			goto out;
+		dirty_bitmap = memslot->dirty_bitmap_head;
+		if (memslot->dirty_bitmap == dirty_bitmap)
+			dirty_bitmap += n / sizeof(long);
 		memset(dirty_bitmap, 0, n);
 
 		r = -ENOMEM;
 		slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
-		if (!slots) {
-			vfree(dirty_bitmap);
+		if (!slots)
 			goto out;
-		}
 		memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
 		slots->memslots[log->slot].dirty_bitmap = dirty_bitmap;
 
@@ -2826,11 +2823,8 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		kfree(old_slots);
 
 		r = -EFAULT;
-		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
-			vfree(dirty_bitmap);
+		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n))
 			goto out;
-		}
-		vfree(dirty_bitmap);
 	} else {
 		r = -EFAULT;
 		if (clear_user(log->dirty_bitmap, n))
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2c62319..199a464 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -118,6 +118,9 @@ struct kvm_memory_slot {
 	unsigned long flags;
 	unsigned long *rmap;
 	unsigned long *dirty_bitmap;
+#ifdef __KVM_DIRTY_LOG_DOUBLE_BUFFERING
+	unsigned long *dirty_bitmap_head;
+#endif
 	struct {
 		unsigned long rmap_pde;
 		int write_count;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e4f5762..6f73b42 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -443,7 +443,10 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
 {
 	if (!memslot->dirty_bitmap)
 		return;
-
+#ifdef __KVM_DIRTY_LOG_DOUBLE_BUFFERING
+	memslot->dirty_bitmap = memslot->dirty_bitmap_head;
+	memslot->dirty_bitmap_head = NULL;
+#endif
 	vfree(memslot->dirty_bitmap);
 	memslot->dirty_bitmap = NULL;
 }
@@ -535,12 +538,17 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
 static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot)
 {
 	unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(memslot);
-
+#ifdef __KVM_DIRTY_LOG_DOUBLE_BUFFERING
+	dirty_bytes *= 2;
+#endif
 	memslot->dirty_bitmap = vmalloc(dirty_bytes);
 	if (!memslot->dirty_bitmap)
 		return -ENOMEM;
 
 	memset(memslot->dirty_bitmap, 0, dirty_bytes);
+#ifdef __KVM_DIRTY_LOG_DOUBLE_BUFFERING
+	memslot->dirty_bitmap_head = memslot->dirty_bitmap;
+#endif
 	return 0;
 }
 
-- 
1.7.0.4

Re: [PATCH 2/2] KVM: x86: pre-allocate one more dirty bitmap to avoid vmalloc() in kvm_vm_ioctl_get_dirty_log()

[Avi Kivity]

On 06/03/2010 04:06 PM, Takuya Yoshikawa wrote:
> Currently x86's kvm_vm_ioctl_get_dirty_log() needs to allocate a bitmap by
> vmalloc() which will be used in the next logging and this has been causing
> bad effects to VGA and live-migration: vmalloc() consumes extra systime,
> triggers tlb flush, etc.
>
> This patch resolves this issue by pre-allocating one more bitmap and switching
> between the two bitmaps during dirty logging.
>
> Performance improvement:
>    In the usual Desktop environment, kvm_vm_ioctl_get_dirty_log() get about
>    twice faster than the original one for graphical updates.
>
>    During live-migration with low work load, the improvement ratio was
>    about three when guest memory was 4GB.
>
>    

Looks good in general.

> Note:
>    Though this patch introduces some ifdefs, we tried not to mixing these
>    with other parts to keep the code as clean as possible.
>
>    

What's the reason for that?  Can't you update the other archs to use 
this as well?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/2] KVM: x86: pre-allocate one more dirty bitmap to avoid vmalloc() in kvm_vm_ioctl_get_dirty_log()

[Takuya Yoshikawa]

On Wed, 09 Jun 2010 16:11:34 +0300
Avi Kivity <avi@redhat.com> wrote:
> 
> Looks good in general.
> 
> > Note:
> >    Though this patch introduces some ifdefs, we tried not to mixing these
> >    with other parts to keep the code as clean as possible.
> >
> >    
> 
> What's the reason for that?  Can't you update the other archs to use 
> this as well?
> 

In general, deleting the #ifdef's will not hurt other archs except for some
extra memory consumption.

- powerpc:

Currently, powerpc updates a dirty log like this:
1. Copy memslot->dirty_bitmap to user space and check dirtiness of the memslot.
2. If memslot was dirty, do some work with page tables.
3. Clear memslot->dirty_bitmap.

So until they begin something like x86's srcu related things, powerpc does not
get enough benefit from one more bitmap space.

- ia64

memslot->dirty_bitmap is only used at the timing of copy_to_user, like
a temporary buffer because ia64 has arch specific bitmap space.

If ia64's dirty logging is important, I would remove this redundancy:
but sadly, ia64 kvm does not seem to be updated constantly and I'm worried
about introducing extra noise without hearing their thought -- If I had
an ia64 box, I might have tried by myself.

Anyway, this will become a completely independent work!

> 
> -- 
> error compiling committee.c: too many arguments to function
> 


-- 
Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>