[Qemu-devel] [PATCH v2] use logging count for individual regions

[Qemu-devel] [PATCH v2] use logging count for individual regions

[Glauber Costa]

qemu-kvm use this scheme of logging count of individual regions,
which is, IMHO, more flexible which the one we have right now.
I'm proposing we use it.

Thanks!

[ v2: follow jan's suggestion and use int type ]

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index f669c3a..4bade66 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -46,6 +46,7 @@ typedef struct KVMSlot
     ram_addr_t phys_offset;
     int slot;
     int flags;
+    int logging_count;
 } KVMSlot;
 
 typedef struct kvm_dirty_log KVMDirtyLog;
@@ -59,7 +60,6 @@ struct KVMState
     int vmfd;
     int coalesced_mmio;
     int broken_set_mem_region;
-    int migration_log;
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
@@ -139,9 +139,7 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     mem.memory_size = slot->memory_size;
     mem.userspace_addr = (unsigned long)qemu_get_ram_ptr(slot->phys_offset);
     mem.flags = slot->flags;
-    if (s->migration_log) {
-        mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
-    }
+
     return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
 }
 
@@ -243,15 +241,22 @@ static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
             return -EINVAL;
     }
 
+    if (flags & KVM_MEM_LOG_DIRTY_PAGES) {
+        if (mem->logging_count++) {
+            return 0;
+        }
+    } else {
+        if (--mem->logging_count) {
+            return 0;
+        }
+    }
+
     old_flags = mem->flags;
 
     flags = (mem->flags & ~mask) | flags;
     mem->flags = flags;
 
     /* If nothing changed effectively, no need to issue ioctl */
-    if (s->migration_log) {
-        flags |= KVM_MEM_LOG_DIRTY_PAGES;
-    }
     if (flags == old_flags) {
             return 0;
     }
@@ -279,8 +284,6 @@ int kvm_set_migration_log(int enable)
     KVMSlot *mem;
     int i, err;
 
-    s->migration_log = enable;
-
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         mem = &s->slots[i];
 
-- 
1.6.2.2

[Qemu-devel] Re: [PATCH v2] use logging count for individual regions

[Jan Kiszka]

Glauber Costa wrote:
> qemu-kvm use this scheme of logging count of individual regions,
> which is, IMHO, more flexible which the one we have right now.
> I'm proposing we use it.
> 
> Thanks!
> 
> [ v2: follow jan's suggestion and use int type ]

Sorry, I did a sloppy review so far:

> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  kvm-all.c |   21 ++++++++++++---------
>  1 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index f669c3a..4bade66 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -46,6 +46,7 @@ typedef struct KVMSlot
>      ram_addr_t phys_offset;
>      int slot;
>      int flags;
> +    int logging_count;
>  } KVMSlot;
>  
>  typedef struct kvm_dirty_log KVMDirtyLog;
> @@ -59,7 +60,6 @@ struct KVMState
>      int vmfd;
>      int coalesced_mmio;
>      int broken_set_mem_region;
> -    int migration_log;
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>      struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
>  #endif
> @@ -139,9 +139,7 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>      mem.memory_size = slot->memory_size;
>      mem.userspace_addr = (unsigned long)qemu_get_ram_ptr(slot->phys_offset);
>      mem.flags = slot->flags;
> -    if (s->migration_log) {
> -        mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
> -    }
> +
>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>  }
>  
> @@ -243,15 +241,22 @@ static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
>              return -EINVAL;
>      }
>  
> +    if (flags & KVM_MEM_LOG_DIRTY_PAGES) {
> +        if (mem->logging_count++) {
> +            return 0;
> +        }
> +    } else {
> +        if (--mem->logging_count) {
> +            return 0;
> +        }
> +    }
> +
>      old_flags = mem->flags;
>  
>      flags = (mem->flags & ~mask) | flags;
>      mem->flags = flags;
>  
>      /* If nothing changed effectively, no need to issue ioctl */
> -    if (s->migration_log) {
> -        flags |= KVM_MEM_LOG_DIRTY_PAGES;
> -    }
>      if (flags == old_flags) {
>              return 0;
>      }

I think kvm_dirty_pages_log_change deserves some refactoring when
switching the logic to a counter. E.g., it makes no sense to pass
flags/mask anymore when we only care about KVM_MEM_LOG_DIRTY_PAGES now
(we practically did the same before, but the internal API was more
powerful). A plain enable bool should suffice.

And this check for (flags == old_flags) will now always be false.

> @@ -279,8 +284,6 @@ int kvm_set_migration_log(int enable)
>      KVMSlot *mem;
>      int i, err;
>  
> -    s->migration_log = enable;
> -
>      for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
>          mem = &s->slots[i];
>  

But this one is more important:

You drop migration_log from kvm_set_migration_log here. And what
increments/decrements logging_count for each active slot now? Please
check this and, well, test the result... :)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux