From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39318) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsCek-0000S3-3N for qemu-devel@nongnu.org; Fri, 30 Oct 2015 12:35:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZsCeg-0004sT-QB for qemu-devel@nongnu.org; Fri, 30 Oct 2015 12:35:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44516) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsCeg-0004sK-Ik for qemu-devel@nongnu.org; Fri, 30 Oct 2015 12:35:54 -0400 Date: Fri, 30 Oct 2015 16:35:49 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20151030163548.GM2417@work-vm> References: <1443515898-3594-1-git-send-email-dgilbert@redhat.com> <1443515898-3594-43-git-send-email-dgilbert@redhat.com> <87611r9s58.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87611r9s58.fsf@neno.neno> Subject: Re: [Qemu-devel] [PATCH v8 42/54] 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, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, bharata@linux.vnet.ibm.com, amit.shah@redhat.com, pbonzini@redhat.com * 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_in_place 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 > > --- > > migration/ram.c | 128 +++++++++++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 103 insertions(+), 25 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > + postcopy_place_needed = false; > > + if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE | > > + RAM_SAVE_FLAG_COMPRESS_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; > > + } > > + page_buffer = host; > > You can move this bit of code here in a different patch, makes review easier. > all_zero can also be on that patch. Done; this is now 'ram_load: Factor out host_from_stream_offset call and check' > > You are reusingh ram_load, but have lots and lots of > > if (postcopy_running) { > > } else { > > } > > I think that it would be easier to just have: > > if (postcopy_running) { > ram_load_postcopy() > } else { > ram_load_precopy{} > } > > You duplicate a bit of code, but remove lots of ifs from the equation, > not sure which one is really easier. I just hate bits like the > following one. > > > @@ -2062,32 +2123,36 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > > } > > break; > > case RAM_SAVE_FLAG_COMPRESS: > > ch = qemu_get_byte(f); > > - ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > > + if (!postcopy_running) { > > + ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > > + } else { > > + memset(page_buffer, ch, TARGET_PAGE_SIZE); > > + if (ch) { > > + all_zero = false; > > + } > > + } > Yeh, I've split that out now into ram_load_postcopy (called from just before the main loop in ram_load); as you say it is a bit bigger, but clearer. > > + if (postcopy_running) { > > > As discussed on irc, I still think that having a RAM_SAVE_HOST_PAGE make > everything much, much clearer and easier, but I agree that is not > trivial with current code. (I've moved this comment down a bit in this reply). Actually, now that the postcopy load code is in a separate routine, it might be possible to reorder things a bit since we know all of these pages are host-page-sized. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK