* Juan Quintela (quint...@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > > > > 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 <dgilb...@redhat.com> > > --- > > 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 / dgilb...@redhat.com / Manchester, UK