From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWCV1-0006TJ-C5 for qemu-devel@nongnu.org; Wed, 29 Jul 2009 12:59:31 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWCUw-0006QQ-Hs for qemu-devel@nongnu.org; Wed, 29 Jul 2009 12:59:30 -0400 Received: from [199.232.76.173] (port=39626 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWCUw-0006QJ-A0 for qemu-devel@nongnu.org; Wed, 29 Jul 2009 12:59:26 -0400 Received: from goliath.siemens.de ([192.35.17.28]:19377) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MWCUv-0005Ip-HF for qemu-devel@nongnu.org; Wed, 29 Jul 2009 12:59:26 -0400 Message-ID: <4A707FE8.1050805@siemens.com> Date: Wed, 29 Jul 2009 18:59:20 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1248884213-22686-1-git-send-email-glommer@redhat.com> In-Reply-To: <1248884213-22686-1-git-send-email-glommer@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v2] use logging count for individual regions List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Glauber Costa Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org 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 > CC: Jan Kiszka > --- > 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