From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54556) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG9Tk-0007U5-Mw for qemu-devel@nongnu.org; Fri, 17 Jul 2015 13:31:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZG9Th-0003P9-Ga for qemu-devel@nongnu.org; Fri, 17 Jul 2015 13:31:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34459) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG9Th-0003Ol-9w for qemu-devel@nongnu.org; Fri, 17 Jul 2015 13:31:17 -0400 Date: Fri, 17 Jul 2015 18:31:10 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150717173109.GN2273@work-vm> References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-35-git-send-email-dgilbert@redhat.com> <87y4iigbpg.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87y4iigbpg.fsf@neno.neno> 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: Juan Quintela Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, liang.z.li@intel.com, "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org, luis@cs.umu.se, amit.shah@redhat.com, pbonzini@redhat.com, david@gibson.dropbear.id.au * Juan Quintela (quintela@redhat.com) wrote: > "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 > > @@ -1742,7 +1752,6 @@ static inline void *host_from_stream_offset(QEMUFile *f, > > error_report("Ack, bad migration stream!"); > > return NULL; > > } > > - > > Dont' belong here O:-) Oops, gone. > > return memory_region_get_ram_ptr(block->mr) + offset; > > } > > > > @@ -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 > > + */ > > + 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; > > NULL, NULL, NULL? Fixed. > BTW, do we really need postcopy_place_source? I think that just doing > s/postcopy_place_source/postcopy_host_page/ would do? They are not always the same. In the host-page size = target-page size case we make use of qemu_get_buffer_in_place(): + qemu_get_buffer_in_place(f, (uint8_t **)&postcopy_place_source, + TARGET_PAGE_SIZE); depending on the alignment of the buffer in the stream that *may* change postcopy_place_source to just point into the qemu_file buffer and then we pluck the data straight out of there without an extra copy. > > 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; > > + } > > + if (!postcopy_running) { > > + page_buffer = host; > > + } else { > > + /* > > + * 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)) { Lets include the next line to make it easier to understand: + all_zero = true; + } > I don't understand the test, the comment or both :-( > > How you arrive from that test that this is a page full of zeros is a > mistery to me :p We end up at this code at the start of each target page received on the stream. That condition is true if we're at the 1st target page within a host page - i.e. the bottom bits (qemu_host_page_mask) of the host address of the page are all zero (the !). When we are at the 1st target page in the host page, we initialise a flag 'all_zero' to be true, which so far must be the case since we've not written anything. If we receive any page that's none zero we clear that flag. If we get to the end of the host-page, find that all_zero is still true, then we can avoid another copy. So the important thing here is that we've not yet convinced ourself that the page is full of zeros; it's just we know we've not read anything in the host page yet, so we assume it's all zeros until we find out otherwise. > Head hurts, would try to convince myself that the rest of changes are ok. Yes, I'll take a week off to recover from the explanation. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK