[Qemu-devel] kvm: savevm is broken for me

[Qemu-devel] kvm: savevm is broken for me

[Luiz Capitulino]

 Hi there,

 If I try to 'savevm' with latest Anthony's tree (HEAD
6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I know
it's not saving anything because vmstat shows no disk activity (which
is the opposite behavior of when 'savevm' works).

 According to 'git bisect' the week's winner is:

"""
commit bd8367761236cd5c435598aeb2f1b8240c09b059
Author: Alexander Graf <agraf@suse.de>
Date:   Fri Jul 17 13:51:48 2009 +0200

    Fake dirty loggin when it's not there
"""

 Indeed, reverting this makes 'savevm' work for me.

[Qemu-devel] Re: kvm: savevm is broken for me

[Alexander Graf]

On 23.07.2009, at 23:58, Luiz Capitulino wrote:

>
> Hi there,
>
> If I try to 'savevm' with latest Anthony's tree (HEAD
> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I know
> it's not saving anything because vmstat shows no disk activity (which
> is the opposite behavior of when 'savevm' works).
>
> According to 'git bisect' the week's winner is:
>
> """
> commit bd8367761236cd5c435598aeb2f1b8240c09b059
> Author: Alexander Graf <agraf@suse.de>
> Date:   Fri Jul 17 13:51:48 2009 +0200
>
>    Fake dirty loggin when it's not there
> """
>
> Indeed, reverting this makes 'savevm' work for me.


Yikes. I have no idea why, but just revert the patch :-). I don't  
really need it anymore since we now have dirty logging in PPC.

Maybe it's the if (is dirty logging really enabled?) in the beginning?  
The rest of the code should be functionally identical. That'd point to  
a bug in the resetting of the DIRTY_ENABLED flag.

Alex

[Qemu-devel] Re: kvm: savevm is broken for me

[Luiz Capitulino]

On Fri, 24 Jul 2009 00:06:13 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
> 
> >
> > Hi there,
> >
> > If I try to 'savevm' with latest Anthony's tree (HEAD
> > 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I know
> > it's not saving anything because vmstat shows no disk activity (which
> > is the opposite behavior of when 'savevm' works).
> >
> > According to 'git bisect' the week's winner is:
> >
> > """
> > commit bd8367761236cd5c435598aeb2f1b8240c09b059
> > Author: Alexander Graf <agraf@suse.de>
> > Date:   Fri Jul 17 13:51:48 2009 +0200
> >
> >    Fake dirty loggin when it's not there
> > """
> >
> > Indeed, reverting this makes 'savevm' work for me.
> 
> 
> Yikes. I have no idea why, but just revert the patch :-). I don't  
> really need it anymore since we now have dirty logging in PPC.
> 
> Maybe it's the if (is dirty logging really enabled?) in the beginning?  

 Yes, it's the if. I have removed it and savevm works.

 Can you submit the fix please?

[Qemu-devel] Re: kvm: savevm is broken for me

[Alexander Graf]

On 24.07.2009, at 15:54, Luiz Capitulino wrote:

> On Fri, 24 Jul 2009 00:06:13 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>
>>>
>>> Hi there,
>>>
>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I know
>>> it's not saving anything because vmstat shows no disk activity  
>>> (which
>>> is the opposite behavior of when 'savevm' works).
>>>
>>> According to 'git bisect' the week's winner is:
>>>
>>> """
>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>> Author: Alexander Graf <agraf@suse.de>
>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>
>>>   Fake dirty loggin when it's not there
>>> """
>>>
>>> Indeed, reverting this makes 'savevm' work for me.
>>
>>
>> Yikes. I have no idea why, but just revert the patch :-). I don't
>> really need it anymore since we now have dirty logging in PPC.
>>
>> Maybe it's the if (is dirty logging really enabled?) in the  
>> beginning?
>
> Yes, it's the if. I have removed it and savevm works.
>
> Can you submit the fix please?

Eh - what fix? You found that something doesn't set or evaluate the  
flags right. That still doesn't tell us who behaves incorrectly.

Alex

[Qemu-devel] Re: kvm: savevm is broken for me

[Luiz Capitulino]

On Fri, 24 Jul 2009 16:18:02 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
> 
> > On Fri, 24 Jul 2009 00:06:13 +0200
> > Alexander Graf <agraf@suse.de> wrote:
> >
> >> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
> >>
> >>>
> >>> Hi there,
> >>>
> >>> If I try to 'savevm' with latest Anthony's tree (HEAD
> >>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I know
> >>> it's not saving anything because vmstat shows no disk activity  
> >>> (which
> >>> is the opposite behavior of when 'savevm' works).
> >>>
> >>> According to 'git bisect' the week's winner is:
> >>>
> >>> """
> >>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
> >>> Author: Alexander Graf <agraf@suse.de>
> >>> Date:   Fri Jul 17 13:51:48 2009 +0200
> >>>
> >>>   Fake dirty loggin when it's not there
> >>> """
> >>>
> >>> Indeed, reverting this makes 'savevm' work for me.
> >>
> >>
> >> Yikes. I have no idea why, but just revert the patch :-). I don't
> >> really need it anymore since we now have dirty logging in PPC.
> >>
> >> Maybe it's the if (is dirty logging really enabled?) in the  
> >> beginning?
> >
> > Yes, it's the if. I have removed it and savevm works.
> >
> > Can you submit the fix please?
> 
> Eh - what fix? You found that something doesn't set or evaluate the  
> flags right. That still doesn't tell us who behaves incorrectly.

 Ah, okay. I thought you meant the 'if' _was_ the bug.

 Better to revert then?

[Qemu-devel] Re: kvm: savevm is broken for me

[Alexander Graf]

On 24.07.2009, at 16:20, Luiz Capitulino wrote:

> On Fri, 24 Jul 2009 16:18:02 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>>
>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>
>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>>
>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>
>>>>>
>>>>> Hi there,
>>>>>
>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I  
>>>>> know
>>>>> it's not saving anything because vmstat shows no disk activity
>>>>> (which
>>>>> is the opposite behavior of when 'savevm' works).
>>>>>
>>>>> According to 'git bisect' the week's winner is:
>>>>>
>>>>> """
>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>
>>>>>  Fake dirty loggin when it's not there
>>>>> """
>>>>>
>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>
>>>>
>>>> Yikes. I have no idea why, but just revert the patch :-). I don't
>>>> really need it anymore since we now have dirty logging in PPC.
>>>>
>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>> beginning?
>>>
>>> Yes, it's the if. I have removed it and savevm works.
>>>
>>> Can you submit the fix please?
>>
>> Eh - what fix? You found that something doesn't set or evaluate the
>> flags right. That still doesn't tell us who behaves incorrectly.
>
> Ah, okay. I thought you meant the 'if' _was_ the bug.
>
> Better to revert then?

Even better yet to find out who's doing something wrong :-).

Alex

Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Glauber Costa]

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

On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>
> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>
>> On Fri, 24 Jul 2009 16:18:02 +0200
>> Alexander Graf <agraf@suse.de> wrote:
>>
>>>
>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>
>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>
>>>>>>
>>>>>> Hi there,
>>>>>>
>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I  
>>>>>> know
>>>>>> it's not saving anything because vmstat shows no disk activity
>>>>>> (which
>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>
>>>>>> According to 'git bisect' the week's winner is:
>>>>>>
>>>>>> """
>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>
>>>>>>  Fake dirty loggin when it's not there
>>>>>> """
>>>>>>
>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>
>>>>>
>>>>> Yikes. I have no idea why, but just revert the patch :-). I don't
>>>>> really need it anymore since we now have dirty logging in PPC.
>>>>>
>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>> beginning?
>>>>
>>>> Yes, it's the if. I have removed it and savevm works.
>>>>
>>>> Can you submit the fix please?
>>>
>>> Eh - what fix? You found that something doesn't set or evaluate the
>>> flags right. That still doesn't tell us who behaves incorrectly.
>>
>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>
>> Better to revert then?
>
> Even better yet to find out who's doing something wrong :-).

It's actually quite simple.
you continue'd instead of break'ing during the dirty log loop,
which would put us in an infinite loop

Patch follows:



[-- Attachment #2: 0001-remove-infinite-loop-for-migration.patch --]
[-- Type: text/plain, Size: 1006 bytes --]

>From d249467b75dea5fad2c305d88edd447e8745494f Mon Sep 17 00:00:00 2001
From: Glauber Costa <glommer@redhat.com>
Date: Fri, 24 Jul 2009 15:32:46 -0400
Subject: [PATCH] remove infinite loop for migration

Linux is so good, that it can finish an endless loop in 5 seconds.
Unfortunately, not everybody uses Linux, and while kvm is still linux-only,
this is likely to change. So remove an endless loop in migration code
recently introduced.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm-all.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Alexander Graf]

On 24.07.2009, at 21:42, Glauber Costa wrote:

> On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>>
>> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>>
>>> On Fri, 24 Jul 2009 16:18:02 +0200
>>> Alexander Graf <agraf@suse.de> wrote:
>>>
>>>>
>>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>>
>>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>>
>>>>>>>
>>>>>>> Hi there,
>>>>>>>
>>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I
>>>>>>> know
>>>>>>> it's not saving anything because vmstat shows no disk activity
>>>>>>> (which
>>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>>
>>>>>>> According to 'git bisect' the week's winner is:
>>>>>>>
>>>>>>> """
>>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>>
>>>>>>> Fake dirty loggin when it's not there
>>>>>>> """
>>>>>>>
>>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>>
>>>>>>
>>>>>> Yikes. I have no idea why, but just revert the patch :-). I don't
>>>>>> really need it anymore since we now have dirty logging in PPC.
>>>>>>
>>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>>> beginning?
>>>>>
>>>>> Yes, it's the if. I have removed it and savevm works.
>>>>>
>>>>> Can you submit the fix please?
>>>>
>>>> Eh - what fix? You found that something doesn't set or evaluate the
>>>> flags right. That still doesn't tell us who behaves incorrectly.
>>>
>>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>>
>>> Better to revert then?
>>
>> Even better yet to find out who's doing something wrong :-).
>
> It's actually quite simple.
> you continue'd instead of break'ing during the dirty log loop,
> which would put us in an infinite loop
>
> Patch follows:

But who calls kvm_physical_sync_dirty_bitmap with !(mem->flags &  
KVM_MEM_LOG_DIRTY_PAGES)? Sounds rather wrong to me.

Alex

Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Glauber Costa]

On Fri, Jul 24, 2009 at 09:45:28PM +0200, Alexander Graf wrote:
>
> On 24.07.2009, at 21:42, Glauber Costa wrote:
>
>> On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>>>
>>> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>>>
>>>> On Fri, 24 Jul 2009 16:18:02 +0200
>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>>
>>>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>>>
>>>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> Hi there,
>>>>>>>>
>>>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I
>>>>>>>> know
>>>>>>>> it's not saving anything because vmstat shows no disk activity
>>>>>>>> (which
>>>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>>>
>>>>>>>> According to 'git bisect' the week's winner is:
>>>>>>>>
>>>>>>>> """
>>>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>>>
>>>>>>>> Fake dirty loggin when it's not there
>>>>>>>> """
>>>>>>>>
>>>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>>>
>>>>>>>
>>>>>>> Yikes. I have no idea why, but just revert the patch :-). I don't
>>>>>>> really need it anymore since we now have dirty logging in PPC.
>>>>>>>
>>>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>>>> beginning?
>>>>>>
>>>>>> Yes, it's the if. I have removed it and savevm works.
>>>>>>
>>>>>> Can you submit the fix please?
>>>>>
>>>>> Eh - what fix? You found that something doesn't set or evaluate the
>>>>> flags right. That still doesn't tell us who behaves incorrectly.
>>>>
>>>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>>>
>>>> Better to revert then?
>>>
>>> Even better yet to find out who's doing something wrong :-).
>>
>> It's actually quite simple.
>> you continue'd instead of break'ing during the dirty log loop,
>> which would put us in an infinite loop
>>
>> Patch follows:
>
> But who calls kvm_physical_sync_dirty_bitmap with !(mem->flags &  
> KVM_MEM_LOG_DIRTY_PAGES)? Sounds rather wrong to me.
I'd have to dig, but from code inspection,
it seems that ram_save_live does it, in its first interaction.

This can probably be improved, but I don't think it is wrong semantics
anyway. We can sync dirty bitmaps without having mem->flags & KVM_MEM_LOG_DIRTY_PAGES,
and just got nothing written to qemu dirty bitmap.

Anyway, even if we decide to fix ram_save_live to not do it,
getting into an endless loop in that case is buggy. So it is really orthogonal.

Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Alexander Graf]

On 24.07.2009, at 22:09, Glauber Costa wrote:

> On Fri, Jul 24, 2009 at 09:45:28PM +0200, Alexander Graf wrote:
>>
>> On 24.07.2009, at 21:42, Glauber Costa wrote:
>>
>>> On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>>>>
>>>> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>>>>
>>>>> On Fri, 24 Jul 2009 16:18:02 +0200
>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>>
>>>>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>>>>
>>>>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi there,
>>>>>>>>>
>>>>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I
>>>>>>>>> know
>>>>>>>>> it's not saving anything because vmstat shows no disk activity
>>>>>>>>> (which
>>>>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>>>>
>>>>>>>>> According to 'git bisect' the week's winner is:
>>>>>>>>>
>>>>>>>>> """
>>>>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>>>>
>>>>>>>>> Fake dirty loggin when it's not there
>>>>>>>>> """
>>>>>>>>>
>>>>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>>>>
>>>>>>>>
>>>>>>>> Yikes. I have no idea why, but just revert the patch :-). I  
>>>>>>>> don't
>>>>>>>> really need it anymore since we now have dirty logging in PPC.
>>>>>>>>
>>>>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>>>>> beginning?
>>>>>>>
>>>>>>> Yes, it's the if. I have removed it and savevm works.
>>>>>>>
>>>>>>> Can you submit the fix please?
>>>>>>
>>>>>> Eh - what fix? You found that something doesn't set or evaluate  
>>>>>> the
>>>>>> flags right. That still doesn't tell us who behaves incorrectly.
>>>>>
>>>>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>>>>
>>>>> Better to revert then?
>>>>
>>>> Even better yet to find out who's doing something wrong :-).
>>>
>>> It's actually quite simple.
>>> you continue'd instead of break'ing during the dirty log loop,
>>> which would put us in an infinite loop
>>>
>>> Patch follows:
>>
>> But who calls kvm_physical_sync_dirty_bitmap with !(mem->flags &
>> KVM_MEM_LOG_DIRTY_PAGES)? Sounds rather wrong to me.
> I'd have to dig, but from code inspection,
> it seems that ram_save_live does it, in its first interaction.
>
> This can probably be improved, but I don't think it is wrong semantics
> anyway. We can sync dirty bitmaps without having mem->flags &  
> KVM_MEM_LOG_DIRTY_PAGES,
> and just got nothing written to qemu dirty bitmap.
>
> Anyway, even if we decide to fix ram_save_live to not do it,
> getting into an endless loop in that case is buggy. So it is really  
> orthogonal.

Looking at the code again it's probably best to remove the if(). Doing  
a break isn't exactly right either because it could hide dirty logs in  
later slots.

Alex

Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Glauber Costa]

On Sat, Jul 25, 2009 at 12:01:59AM +0200, Alexander Graf wrote:
>
> On 24.07.2009, at 22:09, Glauber Costa wrote:
>
>> On Fri, Jul 24, 2009 at 09:45:28PM +0200, Alexander Graf wrote:
>>>
>>> On 24.07.2009, at 21:42, Glauber Costa wrote:
>>>
>>>> On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>>>>>
>>>>> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>>>>>
>>>>>> On Fri, 24 Jul 2009 16:18:02 +0200
>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>>
>>>>>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>>>>>
>>>>>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>
>>>>>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi there,
>>>>>>>>>>
>>>>>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang and I
>>>>>>>>>> know
>>>>>>>>>> it's not saving anything because vmstat shows no disk activity
>>>>>>>>>> (which
>>>>>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>>>>>
>>>>>>>>>> According to 'git bisect' the week's winner is:
>>>>>>>>>>
>>>>>>>>>> """
>>>>>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>>>>>
>>>>>>>>>> Fake dirty loggin when it's not there
>>>>>>>>>> """
>>>>>>>>>>
>>>>>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yikes. I have no idea why, but just revert the patch :-). 
>>>>>>>>> I don't
>>>>>>>>> really need it anymore since we now have dirty logging in PPC.
>>>>>>>>>
>>>>>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>>>>>> beginning?
>>>>>>>>
>>>>>>>> Yes, it's the if. I have removed it and savevm works.
>>>>>>>>
>>>>>>>> Can you submit the fix please?
>>>>>>>
>>>>>>> Eh - what fix? You found that something doesn't set or 
>>>>>>> evaluate the
>>>>>>> flags right. That still doesn't tell us who behaves incorrectly.
>>>>>>
>>>>>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>>>>>
>>>>>> Better to revert then?
>>>>>
>>>>> Even better yet to find out who's doing something wrong :-).
>>>>
>>>> It's actually quite simple.
>>>> you continue'd instead of break'ing during the dirty log loop,
>>>> which would put us in an infinite loop
>>>>
>>>> Patch follows:
>>>
>>> But who calls kvm_physical_sync_dirty_bitmap with !(mem->flags &
>>> KVM_MEM_LOG_DIRTY_PAGES)? Sounds rather wrong to me.
>> I'd have to dig, but from code inspection,
>> it seems that ram_save_live does it, in its first interaction.
>>
>> This can probably be improved, but I don't think it is wrong semantics
>> anyway. We can sync dirty bitmaps without having mem->flags &  
>> KVM_MEM_LOG_DIRTY_PAGES,
>> and just got nothing written to qemu dirty bitmap.
>>
>> Anyway, even if we decide to fix ram_save_live to not do it,
>> getting into an endless loop in that case is buggy. So it is really  
>> orthogonal.
>
> Looking at the code again it's probably best to remove the if(). Doing a 
> break isn't exactly right either because it could hide dirty logs in  
> later slots.
It's up to you.

As an alternative, we could do start_addr = mem->phys_addr + mem->len,
or something like it, therefore skipping the whole slot.

Re: [Qemu-devel] Re: kvm: savevm is broken for me

[Alexander Graf]

On 27.07.2009, at 18:46, Glauber Costa wrote:

> On Sat, Jul 25, 2009 at 12:01:59AM +0200, Alexander Graf wrote:
>>
>> On 24.07.2009, at 22:09, Glauber Costa wrote:
>>
>>> On Fri, Jul 24, 2009 at 09:45:28PM +0200, Alexander Graf wrote:
>>>>
>>>> On 24.07.2009, at 21:42, Glauber Costa wrote:
>>>>
>>>>> On Fri, Jul 24, 2009 at 04:27:38PM +0200, Alexander Graf wrote:
>>>>>>
>>>>>> On 24.07.2009, at 16:20, Luiz Capitulino wrote:
>>>>>>
>>>>>>> On Fri, 24 Jul 2009 16:18:02 +0200
>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On 24.07.2009, at 15:54, Luiz Capitulino wrote:
>>>>>>>>
>>>>>>>>> On Fri, 24 Jul 2009 00:06:13 +0200
>>>>>>>>> Alexander Graf <agraf@suse.de> wrote:
>>>>>>>>>
>>>>>>>>>> On 23.07.2009, at 23:58, Luiz Capitulino wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi there,
>>>>>>>>>>>
>>>>>>>>>>> If I try to 'savevm' with latest Anthony's tree (HEAD
>>>>>>>>>>> 6f725c139ae975646c44ace77bf796318a5783da) QEMU will hang  
>>>>>>>>>>> and I
>>>>>>>>>>> know
>>>>>>>>>>> it's not saving anything because vmstat shows no disk  
>>>>>>>>>>> activity
>>>>>>>>>>> (which
>>>>>>>>>>> is the opposite behavior of when 'savevm' works).
>>>>>>>>>>>
>>>>>>>>>>> According to 'git bisect' the week's winner is:
>>>>>>>>>>>
>>>>>>>>>>> """
>>>>>>>>>>> commit bd8367761236cd5c435598aeb2f1b8240c09b059
>>>>>>>>>>> Author: Alexander Graf <agraf@suse.de>
>>>>>>>>>>> Date:   Fri Jul 17 13:51:48 2009 +0200
>>>>>>>>>>>
>>>>>>>>>>> Fake dirty loggin when it's not there
>>>>>>>>>>> """
>>>>>>>>>>>
>>>>>>>>>>> Indeed, reverting this makes 'savevm' work for me.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yikes. I have no idea why, but just revert the patch :-).
>>>>>>>>>> I don't
>>>>>>>>>> really need it anymore since we now have dirty logging in  
>>>>>>>>>> PPC.
>>>>>>>>>>
>>>>>>>>>> Maybe it's the if (is dirty logging really enabled?) in the
>>>>>>>>>> beginning?
>>>>>>>>>
>>>>>>>>> Yes, it's the if. I have removed it and savevm works.
>>>>>>>>>
>>>>>>>>> Can you submit the fix please?
>>>>>>>>
>>>>>>>> Eh - what fix? You found that something doesn't set or
>>>>>>>> evaluate the
>>>>>>>> flags right. That still doesn't tell us who behaves  
>>>>>>>> incorrectly.
>>>>>>>
>>>>>>> Ah, okay. I thought you meant the 'if' _was_ the bug.
>>>>>>>
>>>>>>> Better to revert then?
>>>>>>
>>>>>> Even better yet to find out who's doing something wrong :-).
>>>>>
>>>>> It's actually quite simple.
>>>>> you continue'd instead of break'ing during the dirty log loop,
>>>>> which would put us in an infinite loop
>>>>>
>>>>> Patch follows:
>>>>
>>>> But who calls kvm_physical_sync_dirty_bitmap with !(mem->flags &
>>>> KVM_MEM_LOG_DIRTY_PAGES)? Sounds rather wrong to me.
>>> I'd have to dig, but from code inspection,
>>> it seems that ram_save_live does it, in its first interaction.
>>>
>>> This can probably be improved, but I don't think it is wrong  
>>> semantics
>>> anyway. We can sync dirty bitmaps without having mem->flags &
>>> KVM_MEM_LOG_DIRTY_PAGES,
>>> and just got nothing written to qemu dirty bitmap.
>>>
>>> Anyway, even if we decide to fix ram_save_live to not do it,
>>> getting into an endless loop in that case is buggy. So it is really
>>> orthogonal.
>>
>> Looking at the code again it's probably best to remove the if().  
>> Doing a
>> break isn't exactly right either because it could hide dirty logs in
>> later slots.
> It's up to you.
>
> As an alternative, we could do start_addr = mem->phys_addr + mem->len,
> or something like it, therefore skipping the whole slot.

The obvious thing would be a goto, but that's kinda icky. I guess  
removing the if() is fine for now.

Alex