Re: [Qemu-devel] [PATCH v7 34/42] Postcopy: Use helpers to map pages during migration

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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" <dgilbert@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_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 <dgilbert@redhat.com>
> 
> 
> > @@ -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