[Qemu-devel] [PATCH] fix endless look at kvm migration

[Qemu-devel] [PATCH] fix endless look at kvm migration

[Glauber Costa]

If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
on a slot, we will enter an endless loop. Remove the test, since it
is not strictly needed anyway

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Alexander Graf <agraf@suse.de>
---
 kvm-all.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 824bb4c..d3bbd3c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -331,11 +331,6 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
             break;
         }
 
-        /* We didn't activate dirty logging? Don't care then. */
-        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
-            continue;
-        }
-
         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
         if (!d.dirty_bitmap) {
             d.dirty_bitmap = qemu_malloc(size);
-- 
1.6.2.2

[Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Alexander Graf]

On 27.07.2009, at 20:26, Glauber Costa wrote:

> If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
> on a slot, we will enter an endless loop. Remove the test, since it
> is not strictly needed anyway

Awesome - thanks a lot!

Acked-by: Alexander Graf <agraf@suse.de>

[Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

Glauber Costa wrote:
> If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
> on a slot, we will enter an endless loop. Remove the test, since it
> is not strictly needed anyway
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Alexander Graf <agraf@suse.de>
> ---
>  kvm-all.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 824bb4c..d3bbd3c 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -331,11 +331,6 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>              break;
>          }
>  
> -        /* We didn't activate dirty logging? Don't care then. */
> -        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
> -            continue;
> -        }
> -
>          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>          if (!d.dirty_bitmap) {
>              d.dirty_bitmap = qemu_malloc(size);

Please let us revert the whole bd83677.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Alexander Graf]

On 27.07.2009, at 20:44, Jan Kiszka wrote:

> Glauber Costa wrote:
>> If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
>> on a slot, we will enter an endless loop. Remove the test, since it
>> is not strictly needed anyway
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>> CC: Alexander Graf <agraf@suse.de>
>> ---
>> kvm-all.c |    5 -----
>> 1 files changed, 0 insertions(+), 5 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 824bb4c..d3bbd3c 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -331,11 +331,6 @@ int  
>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>             break;
>>         }
>>
>> -        /* We didn't activate dirty logging? Don't care then. */
>> -        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>> -            continue;
>> -        }
>> -
>>         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>>         if (!d.dirty_bitmap) {
>>             d.dirty_bitmap = qemu_malloc(size);
>
> Please let us revert the whole bd83677.

Does the non-functional if (r < 0) hurt anyone?

But if you like cleaniness, yes - reverting the whole thing would be  
fine with me too.

Alex

[Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

Alexander Graf wrote:
> 
> On 27.07.2009, at 20:44, Jan Kiszka wrote:
> 
>> Glauber Costa wrote:
>>> If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
>>> on a slot, we will enter an endless loop. Remove the test, since it
>>> is not strictly needed anyway
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>> CC: Alexander Graf <agraf@suse.de>
>>> ---
>>> kvm-all.c |    5 -----
>>> 1 files changed, 0 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 824bb4c..d3bbd3c 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -331,11 +331,6 @@ int
>>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>>             break;
>>>         }
>>>
>>> -        /* We didn't activate dirty logging? Don't care then. */
>>> -        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>>> -            continue;
>>> -        }
>>> -
>>>         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>>>         if (!d.dirty_bitmap) {
>>>             d.dirty_bitmap = qemu_malloc(size);
>>
>> Please let us revert the whole bd83677.
> 
> Does the non-functional if (r < 0) hurt anyone?

I would reduce the motivation of the next kvm-arch contributor to
implement proper dirty log tracking. :)

> 
> But if you like cleaniness, yes - reverting the whole thing would be
> fine with me too.
> 

There are no users for this in sight, so it's best the clean it up - and
then fix the error check that this discussion has revealed.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Glauber Costa]

On Mon, Jul 27, 2009 at 08:44:59PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
> > on a slot, we will enter an endless loop. Remove the test, since it
> > is not strictly needed anyway
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > CC: Alexander Graf <agraf@suse.de>
> > ---
> >  kvm-all.c |    5 -----
> >  1 files changed, 0 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 824bb4c..d3bbd3c 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -331,11 +331,6 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> >              break;
> >          }
> >  
> > -        /* We didn't activate dirty logging? Don't care then. */
> > -        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
> > -            continue;
> > -        }
> > -
> >          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
> >          if (!d.dirty_bitmap) {
> >              d.dirty_bitmap = qemu_malloc(size);
> 
> Please let us revert the whole bd83677.
I'm not opposed either. Avi can surely do that if he prefer.

> 
> Jan
> 

Re: [Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Luiz Capitulino]

On Mon, 27 Jul 2009 17:21:34 -0300
Glauber Costa <glommer@redhat.com> wrote:

> On Mon, Jul 27, 2009 at 08:44:59PM +0200, Jan Kiszka wrote:
> > Glauber Costa wrote:
> > > If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
> > > on a slot, we will enter an endless loop. Remove the test, since it
> > > is not strictly needed anyway
> > > 
> > > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > > CC: Alexander Graf <agraf@suse.de>
> > > ---
> > >  kvm-all.c |    5 -----
> > >  1 files changed, 0 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/kvm-all.c b/kvm-all.c
> > > index 824bb4c..d3bbd3c 100644
> > > --- a/kvm-all.c
> > > +++ b/kvm-all.c
> > > @@ -331,11 +331,6 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> > >              break;
> > >          }
> > >  
> > > -        /* We didn't activate dirty logging? Don't care then. */
> > > -        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
> > > -            continue;
> > > -        }
> > > -
> > >          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
> > >          if (!d.dirty_bitmap) {
> > >              d.dirty_bitmap = qemu_malloc(size);
> > 
> > Please let us revert the whole bd83677.
> I'm not opposed either. Avi can surely do that if he prefer.

 As you have submitted the if() removal fix, you could submit
the revert as well, so that we keep this going.

Re: [Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Glauber Costa]

On Mon, Jul 27, 2009 at 05:22:51PM -0300, Luiz Capitulino wrote:
> On Mon, 27 Jul 2009 17:21:34 -0300
> Glauber Costa <glommer@redhat.com> wrote:
> 
> > On Mon, Jul 27, 2009 at 08:44:59PM +0200, Jan Kiszka wrote:
> > > Glauber Costa wrote:
> > > > If we try to sync bitmaps withouth KVM_MEM_LOG_DIRTY_PAGES being set
> > > > on a slot, we will enter an endless loop. Remove the test, since it
> > > > is not strictly needed anyway
> > > > 
> > > > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > > > CC: Alexander Graf <agraf@suse.de>
> > > > ---
> > > >  kvm-all.c |    5 -----
> > > >  1 files changed, 0 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/kvm-all.c b/kvm-all.c
> > > > index 824bb4c..d3bbd3c 100644
> > > > --- a/kvm-all.c
> > > > +++ b/kvm-all.c
> > > > @@ -331,11 +331,6 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> > > >              break;
> > > >          }
> > > >  
> > > > -        /* We didn't activate dirty logging? Don't care then. */
> > > > -        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
> > > > -            continue;
> > > > -        }
> > > > -
> > > >          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
> > > >          if (!d.dirty_bitmap) {
> > > >              d.dirty_bitmap = qemu_malloc(size);
> > > 
> > > Please let us revert the whole bd83677.
> > I'm not opposed either. Avi can surely do that if he prefer.
> 
>  As you have submitted the if() removal fix, you could submit
> the revert as well, so that we keep this going.
Just that I don't see a point. Reverting a patch is a matter of doing
a "git revert <treeish>"

That's up to the maintainer to do. And btw, I totally forgot that we're
in qemu list here ;-) It's Anthony, not avi.

Re: [Qemu-devel] Re: [PATCH] fix endless look at kvm migration

[Anthony Liguori]

Glauber Costa wrote:
> Just that I don't see a point. Reverting a patch is a matter of doing
> a "git revert <treeish>"
>
> That's up to the maintainer to do. And btw, I totally forgot that we're
> in qemu list here ;-) It's Anthony, not avi.
>   

I already did in my local tree.  Just doing a full rebuild to test...

-- 
Regards,

Anthony Liguori