Re: [Qemu-devel] [PATCH v5 36/45] Postcopy: Use helpers to map pages during migration

[David Gibson]

On Wed, Feb 25, 2015 at 04:51:59PM +0000, Dr. David Alan Gilbert (git) 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.
> 
> Note, gcc 4.9.2 is giving me false uninitialized warnings in ram_load's
> switch, so anything conditionally set at the start of the switch needs
> initializing; filed as
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64614
> 
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  arch_init.c | 137 
> ++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 111 insertions(+), 26 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index acf65e1..7a1c9ea 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1479,9 +1479,39 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, 
> void *host)
>      return 0;
>  }
>  
> +/*
> + * Helper for host_from_stream_offset in the succesful case.
> + * Returns the host pointer for the given block and offset
> + *   calling the postcopy hook and filling in *rb.
> + */
> +static void *host_with_postcopy_hook(MigrationIncomingState *mis,
> +                                     ram_addr_t offset,
> +                                     RAMBlock *block,
> +                                     RAMBlock **rb)
> +{
> +    if (rb) {
> +        *rb = block;
> +    }
> +
> +    postcopy_hook_early_receive(mis,
> +        (offset + block->offset) >> TARGET_PAGE_BITS);

What does postcopy_hook_early_receive() do?

> +    return memory_region_get_ram_ptr(block->mr) + offset;
> +}
> +
> +/*
> + * Read a RAMBlock ID from the stream f, find the host address of the
> + * start of that block and add on 'offset'
> + *
> + * f: Stream to read from
> + * mis: MigrationIncomingState
> + * offset: Offset within the block
> + * flags: Page flags (mostly to see if it's a continuation of previous block)
> + * rb: Pointer to RAMBlock* that gets filled in with the RB we find
> + */
>  static inline void *host_from_stream_offset(QEMUFile *f,
> +                                            MigrationIncomingState *mis,
>                                              ram_addr_t offset,
> -                                            int flags)
> +                                            int flags, RAMBlock **rb)
>  {
>      static RAMBlock *block = NULL;
>      char id[256];
> @@ -1492,8 +1522,7 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>              error_report("Ack, bad migration stream!");
>              return NULL;
>          }
> -
> -        return memory_region_get_ram_ptr(block->mr) + offset;
> +        return host_with_postcopy_hook(mis, offset, block, rb);
>      }
>  
>      len = qemu_get_byte(f);
> @@ -1503,7 +1532,7 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>          if (!strncmp(id, block->idstr, sizeof(id)) &&
>              block->max_length > offset) {
> -            return memory_region_get_ram_ptr(block->mr) + offset;
> +            return host_with_postcopy_hook(mis, offset, block, rb);
>          }
>      }
>  
> @@ -1537,6 +1566,15 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  {
>      int flags = 0, ret = 0;
>      static uint64_t seq_iter;
> +    /*
> +     * 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;
>  
>      seq_iter++;
>  
> @@ -1545,14 +1583,57 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>      }
>  
>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
> +        RAMBlock *rb = 0; /* =0 needed to silence compiler */
>          ram_addr_t addr, total_ram_bytes;
> -        void *host;
> +        void *host = 0;
> +        void *page_buffer = 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, &rb);
> +            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)) {
> +                    all_zero = true;
> +                }
> +
> +                /*
> +                 * If it's the last part of a host page then we place the 
> host
> +                 * page
> +                 */

So it's assumed in a bunch of places that the other end will send
target pages in order until you have a whole host page.  Which is
fine, but it would be nice to have some tests to verify that, just in
case somebody changes that behaviour in future, then wonders why
everything broke.

As I suggested on the earlier patch, I think some of this stuff might
become less clunky, if you had a "receive_host_page" subfunction with
its own inner loop.

> +                postcopy_place_needed = (((uintptr_t)host + 
> TARGET_PAGE_SIZE) &
> +                                         ~qemu_host_page_mask) == 0;
> +            }
> +        } else {
> +            postcopy_place_needed = false;
> +        }
> +
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>          case RAM_SAVE_FLAG_MEM_SIZE:
>              /* Synchronize RAM block list */
> @@ -1590,32 +1671,27 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>              }
>              break;
>          case RAM_SAVE_FLAG_COMPRESS:
> -            host = host_from_stream_offset(f, addr, flags);
> -            if (!host) {
> -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> -                ret = -EINVAL;
> -                break;
> -            }
> -
>              ch = qemu_get_byte(f);
> -            ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
> -            break;
> -        case RAM_SAVE_FLAG_PAGE:
> -            host = host_from_stream_offset(f, addr, flags);
> -            if (!host) {
> -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> -                ret = -EINVAL;
> -                break;
> +            if (!postcopy_running) {
> +                ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
> +            } else {
> +                memset(page_buffer, ch, TARGET_PAGE_SIZE);
> +                if (ch) {
> +                    all_zero = false;

It's a bit nasty that the non-postcopy cases are now out-of-line with
the postcopy cases in-line.  I think it would be nicer to alter the
helper functions so that they work on a supplied buffer, rather than
directly on the ram state.  Then you should be able to use the same
helpers for either case, with just different postcopy logic before and
after the actual page load.

> +                }
>              }
> +            break;
>  
> -            qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +        case RAM_SAVE_FLAG_PAGE:
> +            all_zero = false;
> +            qemu_get_buffer(f, page_buffer, TARGET_PAGE_SIZE);
>              break;
> +
>          case RAM_SAVE_FLAG_XBZRLE:
> -            host = host_from_stream_offset(f, addr, flags);
> -            if (!host) {
> -                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> -                ret = -EINVAL;
> -                break;
> +            all_zero = false;
> +            if (postcopy_running) {
> +                error_report("XBZRLE RAM block in postcopy mode @%zx\n", 
> addr);
> +                return -EINVAL;
>              }
>  
>              if (load_xbzrle(f, addr, host) < 0) {
> @@ -1637,6 +1713,15 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>                  ret = -EINVAL;
>              }
>          }
> +
> +        if (postcopy_place_needed) {
> +            /* This gets called at the last target page in the host page */
> +            ret = postcopy_place_page(mis, host + TARGET_PAGE_SIZE -
> +                                           qemu_host_page_size,
> +                                      postcopy_host_page,
> +                                      (addr + rb->offset) >> 
> TARGET_PAGE_BITS,
> +                                      all_zero);
> +        }
>          if (!ret) {
>              ret = qemu_file_get_error(f);
>          }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpatSIIFP2M2.pgp
Description: PGP signature