[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH v2] use logging count for individual regions
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [PATCH v2] use logging count for individual regions |
Date: |
Wed, 29 Jul 2009 18:59:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
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 <address@hidden>
> CC: Jan Kiszka <address@hidden>
> ---
> 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