[PATCH 0/2] kvm: Some more trivial fixes for clear dirty log

[PATCH 0/2] kvm: Some more trivial fixes for clear dirty log

[Peter Xu]

Two issues I've noticed when I'm drafting the QEMU support of it.
With these two patches applied the QEMU binary (with the clear dirty
log supported [1]) can migrate correctly otherwise the migration can
stall forever if with/after heavy memory workload.

Please have a look, thanks.

[1] https://github.com/xzpeter/qemu/tree/kvm-clear-dirty-log

Peter Xu (2):
  kvm: Fix the bitmap range to copy during clear dirty
  kvm: Fix loop of clear dirty with off-by-one

 virt/kvm/kvm_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/2] kvm: Fix the bitmap range to copy during clear dirty

[Peter Xu]

kvm_dirty_bitmap_bytes() will return the size of the dirty bitmap of
the memslot rather than the size of bitmap passed over from the ioctl.
Here for KVM_CLEAR_DIRTY_LOG we should only copy exactly the size of
bitmap that covers kvm_clear_dirty_log.num_pages.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 53de2f946f9e..ad39c57de82d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1251,7 +1251,7 @@ int kvm_clear_dirty_log_protect(struct kvm *kvm,
 	if (!dirty_bitmap)
 		return -ENOENT;
 
-	n = kvm_dirty_bitmap_bytes(memslot);
+	n = ALIGN(log->num_pages, BITS_PER_LONG) / 8;
 
 	if (log->first_page > memslot->npages ||
 	    log->num_pages > memslot->npages - log->first_page ||
-- 
2.17.1

[PATCH 2/2] kvm: Fix loop of clear dirty with off-by-one

[Peter Xu]

Just imaging the case where num_pages < BITS_PER_LONG, then the loop
will be skipped while it shouldn't.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 virt/kvm/kvm_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ad39c57de82d..15601a19a87a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1264,8 +1264,8 @@ int kvm_clear_dirty_log_protect(struct kvm *kvm,
 		return -EFAULT;
 
 	spin_lock(&kvm->mmu_lock);
-	for (offset = log->first_page,
-	     i = offset / BITS_PER_LONG, n = log->num_pages / BITS_PER_LONG; n--;
+	for (offset = log->first_page, i = offset / BITS_PER_LONG,
+		 n = (log->num_pages / BITS_PER_LONG + 1); n--;
 	     i++, offset += BITS_PER_LONG) {
 		unsigned long mask = *dirty_bitmap_buffer++;
 		atomic_long_t *p = (atomic_long_t *) &dirty_bitmap[i];
-- 
2.17.1

Re: [PATCH 0/2] kvm: Some more trivial fixes for clear dirty log

[Paolo Bonzini]

On 08/05/19 00:44, Peter Xu wrote:
> Two issues I've noticed when I'm drafting the QEMU support of it.
> With these two patches applied the QEMU binary (with the clear dirty
> log supported [1]) can migrate correctly otherwise the migration can
> stall forever if with/after heavy memory workload.
> 
> Please have a look, thanks.
> 
> [1] https://github.com/xzpeter/qemu/tree/kvm-clear-dirty-log
> 
> Peter Xu (2):
>   kvm: Fix the bitmap range to copy during clear dirty
>   kvm: Fix loop of clear dirty with off-by-one
> 
>  virt/kvm/kvm_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Looks good, but this is a blocker for using this feature in userspace.
I think we should change the capability name and number.

Paolo

Re: [PATCH 0/2] kvm: Some more trivial fixes for clear dirty log

[Peter Xu]

On Wed, May 08, 2019 at 10:12:49AM +0200, Paolo Bonzini wrote:
> On 08/05/19 00:44, Peter Xu wrote:
> > Two issues I've noticed when I'm drafting the QEMU support of it.
> > With these two patches applied the QEMU binary (with the clear dirty
> > log supported [1]) can migrate correctly otherwise the migration can
> > stall forever if with/after heavy memory workload.
> > 
> > Please have a look, thanks.
> > 
> > [1] https://github.com/xzpeter/qemu/tree/kvm-clear-dirty-log
> > 
> > Peter Xu (2):
> >   kvm: Fix the bitmap range to copy during clear dirty
> >   kvm: Fix loop of clear dirty with off-by-one
> > 
> >  virt/kvm/kvm_main.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> 
> Looks good, but this is a blocker for using this feature in userspace.
> I think we should change the capability name and number.

Ok, let me add another patch for it.  Also I probably have made a
mistake too in patch 2 (it's not really off-by-one, but it should be a
DIV_ROUND_UP I think...).  I'll post a new version soon.

Thanks,

-- 
Peter Xu