From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47273) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bo7gm-0007Zf-Sf for qemu-devel@nongnu.org; Sun, 25 Sep 2016 07:33:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bo7gi-00030k-2e for qemu-devel@nongnu.org; Sun, 25 Sep 2016 07:33:44 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:2920) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bo7gh-0002wM-4I for qemu-devel@nongnu.org; Sun, 25 Sep 2016 07:33:40 -0400 References: <57D90289.6020003@huawei.com> <57E3D9AF.4060502@huawei.com> From: "Herongguang (Stephen)" Message-ID: <57E7B5FC.7030100@huawei.com> Date: Sun, 25 Sep 2016 19:33:16 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org, quintela@redhat.com, amit.shah@redhat.com Cc: arei.gonglei@huawei.com, "Huangweidong (C)" On 2016/9/23 15:17, Paolo Bonzini wrote: > > > On 22/09/2016 15:16, Herongguang (Stephen) wrote: >> I have some concern: >> 1. For example, vhost does not know about as_id, I wonder if guests in >> SMM can operate disk or ether card, as in >> that case vhost would not logging dirty pages correctly, without knowing >> as_id. > > In the end memory is logged by ram_addr_t, not by address space. So if > vhost_sync_dirty_bitmap is called on the right region everything works. > > Guests in SMM can operate on storage devices, but storage devices cannot > write to 0xA0000-0xBFFFF so that's safe. > >> 2. If a memory region is disabled/enabled/disabled frequently, since >> disabled memory regions would be removed >> from memory slots in kvm-kmod, dirty pages would be discarded in >> kvm-kmod and qemu when disabled, thus missing. >> Is my assumption correct? > > As you found, this is handled by kvm_set_phys_mem: > > if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { > kvm_physical_sync_dirty_bitmap(kml, section); > } > >> 3. I agree your opinion that the right solution is to get dirty-page >> information for all memory region from >> kvm-kmod. But I found it’s somewhat hard to implement since >> kvm_log_sync() expects a MemoryRegionSection* >> parameter. Do you have good idea? > > You're right, memory_region_sync_dirty_bitmap handles this but it's > inefficient. > > So your patch is correct, but it breaks for !x86 as you probably know. > We need to look at the address spaces which have a listener that > implements log_sync. > >> As to all the ram memory regions, I think they are all in the >> ram_list.blocks, so there is no need to create >> a notifier, is this correct? > > Yes, makes sense. > > The patch here is a bit of a hack but is efficient. It looks at > each address space just once and calls log_sync just once per > FlatRange. > > Paolo > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 3e4d416..23b086d 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1154,12 +1154,11 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, > hwaddr addr, uint64_t size); > > /** > - * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory > + * memory_global_dirty_log_sync: synchronize the dirty log for all memory > * > - * Synchronizes the dirty page log for an entire address space. > - * @as: the address space that contains the memory being synchronized > + * Synchronizes the dirty page log for all address spaces. > */ > -void address_space_sync_dirty_bitmap(AddressSpace *as); > +void memory_global_dirty_log_sync(void); > > /** > * memory_region_transaction_begin: Start a transaction. > diff --git a/memory.c b/memory.c > index 1a1baf5..6cac674 100644 > --- a/memory.c > +++ b/memory.c > @@ -158,15 +158,11 @@ static bool memory_listener_match(MemoryListener *listener, > > /* No need to ref/unref .mr, the FlatRange keeps it alive. */ > #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...) \ > - MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) { \ > - .mr = (fr)->mr, \ > - .address_space = (as), \ > - .offset_within_region = (fr)->offset_in_region, \ > - .size = (fr)->addr.size, \ > - .offset_within_address_space = int128_get64((fr)->addr.start), \ > - .readonly = (fr)->readonly, \ > - }), ##_args) > - > + do { \ > + MemoryRegionSection mrs = section_from_flat_range(fr, as); \ > + MEMORY_LISTENER_CALL(callback, dir, &mrs, ##_args); \ > + } while(0) > + > struct CoalescedMemoryRange { > AddrRange addr; > QTAILQ_ENTRY(CoalescedMemoryRange) link; > @@ -245,6 +241,19 @@ typedef struct AddressSpaceOps AddressSpaceOps; > #define FOR_EACH_FLAT_RANGE(var, view) \ > for (var = (view)->ranges; var < (view)->ranges + (view)->nr; ++var) > > +static inline MemoryRegionSection > +section_from_flat_range(FlatRange *fr, AddressSpace *as) > +{ > + return (MemoryRegionSection) { > + .mr = fr->mr, > + .address_space = as, > + .offset_within_region = fr->offset_in_region, > + .size = fr->addr.size, > + .offset_within_address_space = int128_get64(fr->addr.start), > + .readonly = fr->readonly, > + }; > +} > + > static bool flatrange_equal(FlatRange *a, FlatRange *b) > { > return a->mr == b->mr > @@ -2124,16 +2133,25 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr) > return mr && mr != container; > } > > -void address_space_sync_dirty_bitmap(AddressSpace *as) > +void memory_global_dirty_log_sync(void) > { > + MemoryListener *listener; > + AddressSpace *as; > FlatView *view; > FlatRange *fr; > > - view = address_space_get_flatview(as); > - FOR_EACH_FLAT_RANGE(fr, view) { > - MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); > + QTAILQ_FOREACH(listener, &memory_listeners, link) { > + if (!listener->log_sync || !listener->address_space_filter) { > + continue; > + } > + as = listener->address_space_filter; > + view = address_space_get_flatview(as); > + FOR_EACH_FLAT_RANGE(fr, view) { > + MemoryRegionSection mrs = section_from_flat_range(fr, as); > + listener->log_sync(listener, &mrs); > + } > + flatview_unref(view); > } > - flatview_unref(view); > } > > void memory_global_dirty_log_start(void) > diff --git a/migration/ram.c b/migration/ram.c > index a6e1c63..e33ff5e 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -626,7 +626,7 @@ static void migration_bitmap_sync(void) > } > > trace_migration_bitmap_sync_start(); > - address_space_sync_dirty_bitmap(&address_space_memory); > + memory_global_dirty_log_sync(); > > qemu_mutex_lock(&migration_bitmap_mutex); > rcu_read_lock(); > Tested-by: He Rongguang > . >