Re: [PATCH v2] KVM: cleanup {kvm_vm_ioctl, kvm}_get_dirty_log()

2010-03-17 Thread Takuya Yoshikawa 

Xiao Guangrong wrote:
Using bitmap_empty() to see whether memslot-dirty_bitmap is empty 


Changlog:
cleanup x86 specific kvm_vm_ioctl_get_dirty_log() and fix a local
parameter's type address Takuya Yoshikawa's suggestion


Oh, for such a tiny comment.



Signed-off-by: Xiao Guangrong xiaoguangr...@cn.fujitsu.com
---
 arch/x86/kvm/x86.c  |   17 -
 virt/kvm/kvm_main.c |7 ++-
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bcf52d1..e6cbbd4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c


What I said was just you may be able to use bitmap_empty() instead of

 -  for (i = 0; !is_dirty  i  n/sizeof(long); i++)
 -  is_dirty = memslot-dirty_bitmap[i];

for x86's code too, if your patch for kvm_get_dirty_log() was correct.


@@ -2644,22 +2644,17 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
  struct kvm_dirty_log *log)
 {
-   int r, n, i;
+   int r, n, is_dirty = 0;
struct kvm_memory_slot *memslot;
-   unsigned long is_dirty = 0;
unsigned long *dirty_bitmap = NULL;
 
 	mutex_lock(kvm-slots_lock);
 
-	r = -EINVAL;

-   if (log-slot = KVM_MEMORY_SLOTS)
+   r = kvm_get_dirty_log(kvm, log, is_dirty);
+   if (r)
goto out;
 
 	memslot = kvm-memslots-memslots[log-slot];

-   r = -ENOENT;
-   if (!memslot-dirty_bitmap)
-   goto out;
-
n = ALIGN(memslot-npages, BITS_PER_LONG) / 8;
 
 	r = -ENOMEM;

@@ -2668,9 +2663,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
goto out;
memset(dirty_bitmap, 0, n);
 
-	for (i = 0; !is_dirty  i  n/sizeof(long); i++)

-   is_dirty = memslot-dirty_bitmap[i];
-
/* If nothing is dirty, don't bother messing with page tables. */
if (is_dirty) {
struct kvm_memslots *slots, *old_slots;
@@ -2694,8 +2686,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
}
 
 	r = 0;

-   if (copy_to_user(log-dirty_bitmap, dirty_bitmap, n))
-   r = -EFAULT;
+
 out_free:
vfree(dirty_bitmap);
 out:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index bcd08b8..b08a7de 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -767,9 +767,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
struct kvm_dirty_log *log, int *is_dirty)
 {
struct kvm_memory_slot *memslot;
-   int r, i;
-   int n;
-   unsigned long any = 0;
+   int r, n, any = 0;
 
 	r = -EINVAL;

if (log-slot = KVM_MEMORY_SLOTS)
@@ -782,8 +780,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
 
 	n = ALIGN(memslot-npages, BITS_PER_LONG) / 8;
 
-	for (i = 0; !any  i  n/sizeof(long); ++i)

-   any = memslot-dirty_bitmap[i];
+   any = !bitmap_empty(memslot-dirty_bitmap, memslot-npages);
 
 	r = -EFAULT;

if (copy_to_user(log-dirty_bitmap, memslot-dirty_bitmap, n))


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KVM: cleanup {kvm_vm_ioctl, kvm}_get_dirty_log()

2010-03-17 Thread Xiao Guangrong 


Takuya Yoshikawa wrote:

 
 Oh, for such a tiny comment.

Your comment is valuable although it's tiny :-)

 

 
 What I said was just you may be able to use bitmap_empty() instead of
 
 -for (i = 0; !is_dirty  i  n/sizeof(long); i++)
 -is_dirty = memslot-dirty_bitmap[i];
 
 for x86's code too, if your patch for kvm_get_dirty_log() was correct.

While i look into x86's code, i found we can direct call kvm_get_dirty_log()
in kvm_vm_ioctl_get_dirty_log() to remove some unnecessary code, this is a
better cleanup way

Thanks,
Xiao
 
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KVM: cleanup {kvm_vm_ioctl, kvm}_get_dirty_log()

2010-03-17 Thread Takuya Yoshikawa 

Xiao Guangrong wrote:


Takuya Yoshikawa wrote:


Oh, for such a tiny comment.


Your comment is valuable although it's tiny :-)



What I said was just you may be able to use bitmap_empty() instead of


-for (i = 0; !is_dirty  i  n/sizeof(long); i++)
-is_dirty = memslot-dirty_bitmap[i];

for x86's code too, if your patch for kvm_get_dirty_log() was correct.


While i look into x86's code, i found we can direct call kvm_get_dirty_log()
in kvm_vm_ioctl_get_dirty_log() to remove some unnecessary code, this is a
better cleanup way


Ah, probably checking the git log will explain you why it is like that!
Marcelo's work? IIRC.



Thanks,
Xiao
 


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KVM: cleanup {kvm_vm_ioctl, kvm}_get_dirty_log()

2010-03-17 Thread Xiao Guangrong 

Takuya Yoshikawa wrote:

 
 Ah, probably checking the git log will explain you why it is like that!
 Marcelo's work? IIRC.

Oh, i find this commit:

commit 706831a7faec7ac0d3057d20df8234c45bbbc3c5
Author: Marcelo Tosatti mtosa...@redhat.com
Date:   Wed Dec 23 14:35:22 2009 -0200

KVM: use SRCU for dirty log

Signed-off-by: Marcelo Tosatti mtosa...@redhat.com

But i don't know why Marcelo separates kvm_get_dirty_log()'s code
into kvm_vm_ioctl_get_dirty_log(). :-(

Thanks,
Xiao

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KVM: cleanup {kvm_vm_ioctl, kvm}_get_dirty_log()

2010-03-17 Thread Marcelo Tosatti 
On Wed, Mar 17, 2010 at 03:49:19PM +0800, Xiao Guangrong wrote:
 Using bitmap_empty() to see whether memslot-dirty_bitmap is empty 
 
 Changlog:
 cleanup x86 specific kvm_vm_ioctl_get_dirty_log() and fix a local
 parameter's type address Takuya Yoshikawa's suggestion
 
 Signed-off-by: Xiao Guangrong xiaoguangr...@cn.fujitsu.com
 ---
  arch/x86/kvm/x86.c  |   17 -
  virt/kvm/kvm_main.c |7 ++-
  2 files changed, 6 insertions(+), 18 deletions(-)
 
 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
 index bcf52d1..e6cbbd4 100644
 --- a/arch/x86/kvm/x86.c
 +++ b/arch/x86/kvm/x86.c
 @@ -2644,22 +2644,17 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
  int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 struct kvm_dirty_log *log)
  {
 - int r, n, i;
 + int r, n, is_dirty = 0;
   struct kvm_memory_slot *memslot;
 - unsigned long is_dirty = 0;
   unsigned long *dirty_bitmap = NULL;
  
   mutex_lock(kvm-slots_lock);
  
 - r = -EINVAL;
 - if (log-slot = KVM_MEMORY_SLOTS)
 + r = kvm_get_dirty_log(kvm, log, is_dirty);
 + if (r)
   goto out;
  
   memslot = kvm-memslots-memslots[log-slot];
 - r = -ENOENT;
 - if (!memslot-dirty_bitmap)
 - goto out;
 -

Its different because the user copy must be done after the 
SRCU assignment.

 index bcd08b8..b08a7de 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -767,9 +767,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
   struct kvm_dirty_log *log, int *is_dirty)
  {
   struct kvm_memory_slot *memslot;
 - int r, i;
 - int n;
 - unsigned long any = 0;
 + int r, n, any = 0;
  
   r = -EINVAL;
   if (log-slot = KVM_MEMORY_SLOTS)
 @@ -782,8 +780,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
  
   n = ALIGN(memslot-npages, BITS_PER_LONG) / 8;
  
 - for (i = 0; !any  i  n/sizeof(long); ++i)
 - any = memslot-dirty_bitmap[i];
 + any = !bitmap_empty(memslot-dirty_bitmap, memslot-npages);

The opencoded version should be faster in comparison to __bitmap_empty 
because the dirty bitmaps are always unsigned long aligned (and also
there's a function call).

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html