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

[Jan Kiszka <jan.kiszka@siemens.com>]

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