From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49529) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNJFT-0005Ca-Jo for qemu-devel@nongnu.org; Thu, 06 Aug 2015 07:22:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNJFQ-0000Sm-CG for qemu-devel@nongnu.org; Thu, 06 Aug 2015 07:22:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60022) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNJFQ-0000S2-4v for qemu-devel@nongnu.org; Thu, 06 Aug 2015 07:22:08 -0400 Date: Thu, 6 Aug 2015 12:22:01 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150806112201.GC2251@work-vm> References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-35-git-send-email-dgilbert@redhat.com> <20150727073934.GE12267@grmbl.mre> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150727073934.GE12267@grmbl.mre> Subject: Re: [Qemu-devel] [PATCH v7 34/42] Postcopy: Use helpers to map pages during migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, pbonzini@redhat.com, david@gibson.dropbear.id.au * Amit Shah (amit.shah@redhat.com) wrote: > On (Tue) 16 Jun 2015 [11:26:47], Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > In postcopy, the destination guest is running at the same time > > as it's receiving pages; as we receive new pages we must put > > them into the guests address space atomically to avoid a running > > CPU accessing a partially written page. > > > > Use the helpers in postcopy-ram.c to map these pages. > > > > qemu_get_buffer_less_copy is used to avoid a copy out of qemu_file > > in the case that postcopy is going to do a copy anyway. > > > > Signed-off-by: Dr. David Alan Gilbert > > > > @@ -1881,6 +1890,16 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > int flags = 0, ret = 0; > > static uint64_t seq_iter; > > int len = 0; > > + /* > > + * System is running in postcopy mode, page inserts to host memory must be > > + * atomic > > + */ > > *If* system is running in postcopy mode .... Done. > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > + bool postcopy_running = postcopy_state_get(mis) >= > > + POSTCOPY_INCOMING_LISTENING; > > + void *postcopy_host_page = NULL; > > + bool postcopy_place_needed = false; > > + bool matching_page_sizes = qemu_host_page_size == TARGET_PAGE_SIZE; > > > > seq_iter++; > > > > @@ -1896,13 +1915,57 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > rcu_read_lock(); > > while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { > > ram_addr_t addr, total_ram_bytes; > > - void *host; > > + void *host = 0; > > + void *page_buffer = 0; > > + void *postcopy_place_source = 0; > > uint8_t ch; > > + bool all_zero = false; > > > > addr = qemu_get_be64(f); > > flags = addr & ~TARGET_PAGE_MASK; > > addr &= TARGET_PAGE_MASK; > > > > + if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE | > > + RAM_SAVE_FLAG_XBZRLE)) { > > + host = host_from_stream_offset(f, mis, addr, flags); > > + if (!host) { > > + error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); > > + ret = -EINVAL; > > + break; > > + } > > So the host_from_stream_offset was moved here from below. One > invocation below is still left, which is a bug.. Thanks, fixed. > > + if (!postcopy_running) { > > + page_buffer = host; > > + } else { > > Instead of this, can we just do: > > page_buffer = host; > if (postcopy_running) { done. > > + /* > > + * Postcopy requires that we place whole host pages atomically. > > + * To make it atomic, the data is read into a temporary page > > + * that's moved into place later. > > + * The migration protocol uses, possibly smaller, target-pages > > + * however the source ensures it always sends all the components > > + * of a host page in order. > > + */ > > + if (!postcopy_host_page) { > > + postcopy_host_page = postcopy_get_tmp_page(mis); > > + } > > + page_buffer = postcopy_host_page + > > + ((uintptr_t)host & ~qemu_host_page_mask); > > + /* If all TP are zero then we can optimise the place */ > > + if (!((uintptr_t)host & ~qemu_host_page_mask)) { > > + all_zero = true; > > + } > > + > > + /* > > + * If it's the last part of a host page then we place the host > > + * page > > + */ > > + postcopy_place_needed = (((uintptr_t)host + TARGET_PAGE_SIZE) & > > + ~qemu_host_page_mask) == 0; > > + postcopy_place_source = postcopy_host_page; > > + } > > + } else { > > + postcopy_place_needed = false; > > + } > > ... and similar for postcopy_place_needed as well? It becomes much > easier to read. Done; actually it's just not needed at all - the function entry initialisation of that flag is sufficient. > > case RAM_SAVE_FLAG_COMPRESS_PAGE: > > - host = host_from_stream_offset(f, addr, flags); > > + all_zero = false; > > + if (postcopy_running) { > > + error_report("Compressed RAM in postcopy mode @%zx\n", addr); > > + return -EINVAL; > > + } > > + host = host_from_stream_offset(f, mis, addr, flags); > > This line should go (as mentioned above)? Yes, done. Dave > > > Amit -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK