[PATCH 0/2] KVM: cleanup: get_dirty_log

[PATCH 0/2] KVM: cleanup: get_dirty_log

[Takuya Yoshikawa]

This is the 2nd version of get_dirty_log cleanup.

Changelog:
  In version 1, I changed the timing of copy_to_user() in the
  powerpc's get_dirty_log by mistake. This time, I've kept the
  timing and tests on ppc box now look OK to me!

Takuya

[PATCH 1/2] KVM: cleanup: open-code kvm_get_dirty_log()

[Takuya Yoshikawa]

kvm_get_dirty_log() is used by ia64 and powerpc, but not used by x86 anymore.
Furtheremore, in the case of ia64, the sanity checks in it have to be done
before kvm_ia64_sync_dirty_log(), resulting in duplicate checks.

So we open-code kvm_get_dirty_log() into arch's get_dirty_log.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/ia64/kvm/kvm-ia64.c  |   15 ++++++++++-----
 arch/powerpc/kvm/book3s.c |   25 ++++++++++++++++++-------
 include/linux/kvm_host.h  |    2 --
 virt/kvm/kvm_main.c       |   34 ----------------------------------
 4 files changed, 28 insertions(+), 48 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index f56a631..c3754bb 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1818,10 +1818,10 @@ static void kvm_ia64_sync_dirty_log(struct kvm *kvm,
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		struct kvm_dirty_log *log)
 {
-	int r;
+	int r, i;
 	unsigned long n;
 	struct kvm_memory_slot *memslot;
-	int is_dirty = 0;
+	unsigned long is_dirty = 0;
 
 	mutex_lock(&kvm->slots_lock);
 
@@ -1835,14 +1835,19 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		goto out;
 
 	kvm_ia64_sync_dirty_log(kvm, memslot);
-	r = kvm_get_dirty_log(kvm, log, &is_dirty);
-	if (r)
+
+	n = kvm_dirty_bitmap_bytes(memslot);
+
+	for (i = 0; !is_dirty && i < n/sizeof(long); ++i)
+		is_dirty = memslot->dirty_bitmap[i];
+
+	r = -EFAULT;
+	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
 		goto out;
 
 	/* If nothing is dirty, don't bother messing with page tables. */
 	if (is_dirty) {
 		kvm_flush_remote_tlbs(kvm);
-		n = kvm_dirty_bitmap_bytes(memslot);
 		memset(memslot->dirty_bitmap, 0, n);
 	}
 	r = 0;
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 7656b6d..691ac31 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1245,27 +1245,38 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	struct kvm_memory_slot *memslot;
 	struct kvm_vcpu *vcpu;
 	ulong ga, ga_end;
-	int is_dirty = 0;
-	int r;
+	unsigned long is_dirty = 0;
+	int r, i;
 	unsigned long n;
 
 	mutex_lock(&kvm->slots_lock);
 
-	r = kvm_get_dirty_log(kvm, log, &is_dirty);
-	if (r)
+	r = -EINVAL;
+	if (log->slot >= KVM_MEMORY_SLOTS)
+		goto out;
+
+	memslot = &kvm->memslots->memslots[log->slot];
+	r = -ENOENT;
+	if (!memslot->dirty_bitmap)
+		goto out;
+
+	n = kvm_dirty_bitmap_bytes(memslot);
+
+	for (i = 0; !is_dirty && i < n/sizeof(long); ++i)
+		is_dirty = memslot->dirty_bitmap[i];
+
+	r = -EFAULT;
+	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
 		goto out;
 
 	/* If nothing is dirty, don't bother messing with page tables. */
 	if (is_dirty) {
-		memslot = &kvm->memslots->memslots[log->slot];
-
 		ga = memslot->base_gfn << PAGE_SHIFT;
 		ga_end = ga + (memslot->npages << PAGE_SHIFT);
 
 		kvm_for_each_vcpu(n, vcpu, kvm)
 			kvmppc_mmu_pte_pflush(vcpu, ga, ga_end);
 
-		n = kvm_dirty_bitmap_bytes(memslot);
 		memset(memslot->dirty_bitmap, 0, n);
 	}
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f2ecdd5..91ee311 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -342,8 +342,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 int kvm_dev_ioctl_check_extension(long ext);
 
-int kvm_get_dirty_log(struct kvm *kvm,
-			struct kvm_dirty_log *log, int *is_dirty);
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 				struct kvm_dirty_log *log);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9a73b98..39b4512 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -776,40 +776,6 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
 
-int kvm_get_dirty_log(struct kvm *kvm,
-			struct kvm_dirty_log *log, int *is_dirty)
-{
-	struct kvm_memory_slot *memslot;
-	int r, i;
-	unsigned long n;
-	unsigned long any = 0;
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
-	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
-
-	n = kvm_dirty_bitmap_bytes(memslot);
-
-	for (i = 0; !any && i < n/sizeof(long); ++i)
-		any = memslot->dirty_bitmap[i];
-
-	r = -EFAULT;
-	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
-		goto out;
-
-	if (any)
-		*is_dirty = 1;
-
-	r = 0;
-out:
-	return r;
-}
-
 void kvm_disable_largepages(void)
 {
 	largepages_enabled = false;
-- 
1.7.0.4

[PATCH 2/2] KVM: cleanup: centralize get_dirty_log ioctl

[Takuya Yoshikawa]

We move sanity check and lock related parts to the arch independent code.
This will help future cleanups.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/ia64/kvm/kvm-ia64.c  |   14 ++------------
 arch/powerpc/kvm/book3s.c |   14 ++------------
 arch/powerpc/kvm/booke.c  |    2 +-
 arch/s390/kvm/kvm-s390.c  |    4 ++--
 arch/x86/kvm/x86.c        |   14 ++------------
 include/linux/kvm_host.h  |    4 ++--
 virt/kvm/kvm_main.c       |   24 ++++++++++++++++++++++++
 7 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index c3754bb..72efc39 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1815,24 +1815,15 @@ static void kvm_ia64_sync_dirty_log(struct kvm *kvm,
 	spin_unlock(&kvm->arch.dirty_log_lock);
 }
 
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-		struct kvm_dirty_log *log)
+int kvm_arch_get_dirty_log(struct kvm *kvm,
+			   struct kvm_dirty_log *log)
 {
 	int r, i;
 	unsigned long n;
 	struct kvm_memory_slot *memslot;
 	unsigned long is_dirty = 0;
 
-	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
 	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
 
 	kvm_ia64_sync_dirty_log(kvm, memslot);
 
@@ -1852,7 +1843,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	}
 	r = 0;
 out:
-	mutex_unlock(&kvm->slots_lock);
 	return r;
 }
 
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 691ac31..f9b1290 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1239,8 +1239,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 /*
  * Get (and clear) the dirty memory log for a memory slot.
  */
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-				      struct kvm_dirty_log *log)
+int kvm_arch_get_dirty_log(struct kvm *kvm,
+			   struct kvm_dirty_log *log)
 {
 	struct kvm_memory_slot *memslot;
 	struct kvm_vcpu *vcpu;
@@ -1249,16 +1249,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	int r, i;
 	unsigned long n;
 
-	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
 	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
 
 	n = kvm_dirty_bitmap_bytes(memslot);
 
@@ -1282,7 +1273,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 
 	r = 0;
 out:
-	mutex_unlock(&kvm->slots_lock);
 	return r;
 }
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index c604277..5eb64b9 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -596,7 +596,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 	return r;
 }
 
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
+int kvm_arch_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 {
 	return -ENOTSUPP;
 }
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 4fe6865..09b7885 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -134,8 +134,8 @@ int kvm_dev_ioctl_check_extension(long ext)
 /*
  * Get (and clear) the dirty memory log for a memory slot.
  */
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-			       struct kvm_dirty_log *log)
+int kvm_arch_get_dirty_log(struct kvm *kvm,
+			   struct kvm_dirty_log *log)
 {
 	return 0;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e6659d..ecb5350 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3019,24 +3019,15 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
 /*
  * Get (and clear) the dirty memory log for a memory slot.
  */
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-				      struct kvm_dirty_log *log)
+int kvm_arch_get_dirty_log(struct kvm *kvm,
+			   struct kvm_dirty_log *log)
 {
 	int r, i;
 	struct kvm_memory_slot *memslot;
 	unsigned long n;
 	unsigned long is_dirty = 0;
 
-	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
 	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
 
 	n = kvm_dirty_bitmap_bytes(memslot);
 
@@ -3087,7 +3078,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 
 	r = 0;
 out:
-	mutex_unlock(&kvm->slots_lock);
 	return r;
 }
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 91ee311..0773d56 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -342,8 +342,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 int kvm_dev_ioctl_check_extension(long ext);
 
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-				struct kvm_dirty_log *log);
+int kvm_arch_get_dirty_log(struct kvm *kvm,
+			   struct kvm_dirty_log *log);
 
 int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   struct
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 39b4512..cdfe32a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -776,6 +776,30 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
 
+static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				      struct kvm_dirty_log *log)
+{
+	struct kvm_memory_slot *memslot;
+	int r;
+
+	mutex_lock(&kvm->slots_lock);
+
+	r = -EINVAL;
+	if (log->slot >= KVM_MEMORY_SLOTS)
+		goto out;
+
+	memslot = &kvm->memslots->memslots[log->slot];
+	r = -ENOENT;
+	if (!memslot->dirty_bitmap)
+		goto out;
+
+	r = kvm_arch_get_dirty_log(kvm, log);
+
+out:
+	mutex_unlock(&kvm->slots_lock);
+	return r;
+}
+
 void kvm_disable_largepages(void)
 {
 	largepages_enabled = false;
-- 
1.7.0.4

Re: [PATCH 0/2] KVM: cleanup: get_dirty_log

[Alexander Graf]

On 03.09.2010, at 10:34, Takuya Yoshikawa wrote:

> This is the 2nd version of get_dirty_log cleanup.
> 
> Changelog:
>  In version 1, I changed the timing of copy_to_user() in the
>  powerpc's get_dirty_log by mistake. This time, I've kept the
>  timing and tests on ppc box now look OK to me!

I seem to get a pretty big chunk of false positives with this set applied. Qemu's vnc server tries to be clever and searches for real changes inside the pages it finds dirty, so the only perceptible thing there is an increased amount of CPU being wasted.

In MOL instead, the VNC server just directly sends pages that are marked dirty. And I get a full new screen update on every update cycle.

Please check with some debugging code if the dirty region really from the dirty bitmap is really only the one that was updated :).


Thank you,

Alex

Re: [PATCH 0/2] KVM: cleanup: get_dirty_log

[Takuya Yoshikawa]

(2010/09/04 18:24), Alexander Graf wrote:
>
> On 03.09.2010, at 10:34, Takuya Yoshikawa wrote:
>
>> This is the 2nd version of get_dirty_log cleanup.
>>
>> Changelog:
>>   In version 1, I changed the timing of copy_to_user() in the
>>   powerpc's get_dirty_log by mistake. This time, I've kept the
>>   timing and tests on ppc box now look OK to me!
>
> I seem to get a pretty big chunk of false positives with this set applied. Qemu's vnc server tries to be clever and searches for real changes inside the pages it finds dirty, so the only perceptible thing there is an increased amount of CPU being wasted.
>
> In MOL instead, the VNC server just directly sends pages that are marked dirty. And I get a full new screen update on every update cycle.
>
> Please check with some debugging code if the dirty region really from the dirty bitmap is really only the one that was updated :).
>

Interesting behavior! This functionality might be more sensitive than I imagined.

I'll investigate with some debugging code as you suggest!

Thank you,
   Takuya