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

[Dr. David Alan Gilbert]

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