[Qemu-devel] [PATCH] kvm: fix kvm_set_migration_log() behavior

[Qemu-devel] [PATCH] kvm: fix kvm_set_migration_log() behavior

[Vincent KHERBACHE]

The test (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) is wrong because 
the condition is valid when enable = 0 and current dirty log memory flag is set.
As a consequence kvm_log_global_stop() does not stop the KVM dirty log
tracking: kvm_set_migration_log(0) didn't do its job.
So instead I propose to use kvm_slot_dirty_pages_log_change() which correctly
compare the memory flags (old/new).

Signed-off-by: Vincent KHERBACHE <vincent.kherbache@inria.fr>
---
 kvm-all.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 2ca9143..f104f87 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -355,7 +355,7 @@ static int kvm_set_migration_log(int enable)
 {
     KVMState *s = kvm_state;
     KVMSlot *mem;
-    int i, err;
+    int i, err = 0;
 
     s->migration_log = enable;
 
@@ -365,15 +365,9 @@ static int kvm_set_migration_log(int enable)
         if (!mem->memory_size) {
             continue;
         }
-        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
-            continue;
-        }
-        err = kvm_set_user_memory_region(s, mem);
-        if (err) {
-            return err;
-        }
+        err = kvm_slot_dirty_pages_log_change(mem, (bool)enable);
     }
-    return 0;
+    return err;
 }
 
 /* get kvm's dirty pages bitmap and update qemu's */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] kvm: fix kvm_set_migration_log() behavior

[Eric Blake]

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

On 02/18/2014 05:33 AM, Vincent KHERBACHE wrote:
> The test (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) is wrong because 
> the condition is valid when enable = 0 and current dirty log memory flag is set.
> As a consequence kvm_log_global_stop() does not stop the KVM dirty log
> tracking: kvm_set_migration_log(0) didn't do its job.
> So instead I propose to use kvm_slot_dirty_pages_log_change() which correctly
> compare the memory flags (old/new).
> 
> Signed-off-by: Vincent KHERBACHE <vincent.kherbache@inria.fr>
> ---
>  kvm-all.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 

> -            return err;
> -        }
> +        err = kvm_slot_dirty_pages_log_change(mem, (bool)enable);

Is this a v2 post?  Any reason you reposted without addressing my
earlier review?
https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02840.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] kvm: fix kvm_set_migration_log() behavior

[Vincent KHERBACHE]

Le 18/02/2014 14:23, Eric Blake a écrit :
>> -            return err;
>> -        }
>> +        err = kvm_slot_dirty_pages_log_change(mem, (bool)enable);
> 
> Is this a v2 post?  Any reason you reposted without addressing my
> earlier review?
> https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02840.html

My bad, I apologize for the duplicate post I just missed your review.

>> +        err = kvm_slot_dirty_pages_log_change(mem, (bool)enable);
>
> Casting to bool looks odd.  We already require a compliant C99 compiler,
> which means the compiler already properly handles the squashing of all
> non-zero values to true when calling a function with a parameter
> prototyped as bool, without needing the cast.

Thanks for information, here is the patch without the useless casting.

BR.

Signed-off-by: Vincent KHERBACHE <vincent.kherbache@inria.fr>
---
 kvm-all.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 2ca9143..007df56 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -355,7 +355,7 @@ static int kvm_set_migration_log(int enable)
 {
     KVMState *s = kvm_state;
     KVMSlot *mem;
-    int i, err;
+    int i, err = 0;

     s->migration_log = enable;

@@ -365,15 +365,9 @@ static int kvm_set_migration_log(int enable)
         if (!mem->memory_size) {
             continue;
         }
-        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
-            continue;
-        }
-        err = kvm_set_user_memory_region(s, mem);
-        if (err) {
-            return err;
-        }
+        err = kvm_slot_dirty_pages_log_change(mem, enable);
     }
-    return 0;
+    return err;
 }

 /* get kvm's dirty pages bitmap and update qemu's */
---
Vincent KHERBACHE

Re: [Qemu-devel] [PATCH] kvm: fix kvm_set_migration_log() behavior

[Paolo Bonzini]

Il 18/02/2014 13:33, Vincent KHERBACHE ha scritto:
> diff --git a/kvm-all.c b/kvm-all.c
> index 2ca9143..f104f87 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -355,7 +355,7 @@ static int kvm_set_migration_log(int enable)
>  {
>      KVMState *s = kvm_state;
>      KVMSlot *mem;
> -    int i, err;
> +    int i, err = 0;
>
>      s->migration_log = enable;
>
> @@ -365,15 +365,9 @@ static int kvm_set_migration_log(int enable)
>          if (!mem->memory_size) {
>              continue;
>          }
> -        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
> -            continue;
> -        }
> -        err = kvm_set_user_memory_region(s, mem);
> -        if (err) {
> -            return err;
> -        }
> +        err = kvm_slot_dirty_pages_log_change(mem, (bool)enable);
>      }
> -    return 0;
> +    return err;
>  }

This patch is not correct, because it introduces another one: if dirty 
logging was enabled on the memory region (kvm_dirty_pages_log_change), 
it will be disabled after migration.

Can you explain better the problem?  I cannot see anything wrong in the 
code, though it is not the clearest.  Note that the effects of 
kvm_set_migration_log and kvm_dirty_pages_log_change should be logically 
ORed.

Paolo

Re: [Qemu-devel] [PATCH] kvm: fix kvm_set_migration_log() behavior

[Vincent KHERBACHE]

Le 18/02/2014 17:16, Paolo Bonzini a écrit :
> This patch is not correct, because it introduces another one: if dirty
> logging was enabled on the memory region (kvm_dirty_pages_log_change),
> it will be disabled after migration.

Yes you're right that was my initial goal, I didn't distinguish the two
cases.

> Note that the effects of
> kvm_set_migration_log and kvm_dirty_pages_log_change should be logically
> ORed.

OK, but that's not really the case (see next comment)

> Can you explain better the problem?  I cannot see anything wrong in the
> code, though it is not the clearest. 

The problem is that if dirty logging is enabled by
kvm_set_migration_log(1) it will NOT be disabled by a
kvm_set_migration_log(0).

So, if this behavior is 'normal', kvm_log_global_stop (which call
kvm_set_migration_log) can not be used independently ?

Also, what I don't understand is why kvm_log_global_start/stop and
kvm_log_start/stop are so different: kvm_set_migration_log VS
kvm_dirty_pages_log_change.


Thank you for the support.

Regards,

-- 
Vincent KHERBACHE