Re: [Qemu-devel] [PATCH v4 40/47] Postcopy: Use helpers to map pages during migration

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

* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Fri, Oct 03, 2014 at 06:47:46PM +0100, 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.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  arch_init.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 87 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch_init.c b/arch_init.c
> > index 2f4345a..0ba627b 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -1458,9 +1458,20 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
> >      return 0;
> >  }
> >  
> > +/*
> > + * 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];
> > @@ -1471,8 +1482,11 @@ static inline void *host_from_stream_offset(QEMUFile *f,
> >              error_report("Ack, bad migration stream!");
> >              return NULL;
> >          }
> > +        if (rb) {
> > +            *rb = block;
> > +        }
> >  
> > -        return memory_region_get_ram_ptr(block->mr) + offset;
> > +        goto gotit;
> 
> This is an ugly use of goto - it looks kind of like the exception
> handling goto idiom, but it's not.  I think it would be nicer to make
> the code fragment after gotit into a helper function.

Indeed; I've added the helper.

> >      }
> >  
> >      len = qemu_get_byte(f);
> > @@ -1480,12 +1494,22 @@ static inline void *host_from_stream_offset(QEMUFile *f,
> >      id[len] = 0;
> >  
> >      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> > -        if (!strncmp(id, block->idstr, sizeof(id)))
> > -            return memory_region_get_ram_ptr(block->mr) + offset;
> > +        if (!strncmp(id, block->idstr, sizeof(id))) {
> > +            if (rb) {
> > +                *rb = block;
> > +            }
> > +            goto gotit;
> > +        }
> >      }
> >  
> >      error_report("Can't find block %s!", id);
> >      return NULL;
> > +
> > +gotit:
> > +    postcopy_hook_early_receive(mis,
> > +        (offset + (*rb)->offset) >> TARGET_PAGE_BITS);
> > +    return memory_region_get_ram_ptr(block->mr) + offset;
> > +
> >  }
> >  
> >  /*
> > @@ -1515,6 +1539,13 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >      ram_addr_t addr;
> >      int flags, 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 = mis->postcopy_ram_state >=
> > +                            POSTCOPY_RAM_INCOMING_LISTENING;
> >  
> >      seq_iter++;
> >  
> > @@ -1523,6 +1554,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >      }
> >  
> >      while (!ret) {
> > +        RAMBlock *rb = 0; /* =0 needed to silence compiler */
> >          addr = qemu_get_be64(f);
> >  
> >          flags = addr & ~TARGET_PAGE_MASK;
> > @@ -1570,7 +1602,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >              void *host;
> >              uint8_t ch;
> >  
> > -            host = host_from_stream_offset(f, addr, flags);
> > +            host = host_from_stream_offset(f, mis, addr, flags, &rb);
> >              if (!host) {
> >                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> >                  ret = -EINVAL;
> > @@ -1578,20 +1610,66 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >              }
> >  
> >              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 {
> > +                if (!ch) {
> > +                    ret = postcopy_place_zero_page(mis, host,
> > +                              (addr + rb->offset) >> TARGET_PAGE_BITS);
> > +                } else {
> > +                    void *tmp;
> > +                    tmp = postcopy_get_tmp_page(mis, (addr + rb->offset) >>
> > +                                                      TARGET_PAGE_BITS);
> > +
> > +                    if (!tmp) {
> > +                        return -ENOMEM;
> > +                    }
> > +                    memset(tmp, ch, TARGET_PAGE_SIZE);
> > +                    ret = postcopy_place_page(mis, host, tmp,
> > +                              (addr + rb->offset) >> TARGET_PAGE_BITS);
> > +                }
> > +                if (ret) {
> > +                    error_report("ram_load: Failure in postcopy compress @"
> > +                                 "%zx/%p;%s+%zx",
> > +                                 addr, host, rb->idstr, rb->offset);
> > +                    return ret;
> > +                }
> > +            }
> 
> Might be nicer to fold this logic into ram_handle_compressed(), since
> there's no obvious reason it should not be used for the postcopy path.

Hmm, that would be true, except ram_handle_compressed is also called from
the RDMA code (that postcopy doesn't yet support) and when it does it's
data path might be a bit different as well.

> >          } else if (flags & RAM_SAVE_FLAG_PAGE) {
> >              void *host;
> >  
> > -            host = host_from_stream_offset(f, addr, flags);
> > +            host = host_from_stream_offset(f, mis, addr, flags, &rb);
> >              if (!host) {
> >                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> >                  ret = -EINVAL;
> >                  break;
> >              }
> >  
> > -            qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > +            if (!postcopy_running) {
> > +                qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > +            } else {
> > +                void *tmp = postcopy_get_tmp_page(mis, (addr + rb->offset) >>
> > +                                                        TARGET_PAGE_BITS);
> > +
> > +                if (!tmp) {
> > +                    return -ENOMEM;
> > +                }
> > +                qemu_get_buffer(f, tmp, TARGET_PAGE_SIZE);
> > +                ret = postcopy_place_page(mis, host, tmp,
> > +                          (addr + rb->offset) >> TARGET_PAGE_BITS);
> > +                if (ret) {
> > +                    error_report("ram_load: Failure in postcopy simple"
> > +                                 "@%zx/%p;%s+%zx",
> > +                                 addr, host, rb->idstr, rb->offset);
> > +                    return ret;
> > +                }
> > +            }
> >          } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
> > -            void *host = host_from_stream_offset(f, addr, flags);
> > +            if (postcopy_running) {
> > +                error_report("XBZRLE RAM block in postcopy mode @%zx\n", addr);
> > +                return -EINVAL;
> > +            }
> 
> Hrm, there doesn't seem like an inherent reason XBZRLE shouldn't be
> possible in postcopy.  Obviously a temporary buffer would be
> necessary.

This is only disabling it in the postcopy stage; so the precopy stage at the beginning
still uses it.   In postcopy, we only ever send a page once, so we won't be sending
the page and then sending an XBZRLE fixup for it.   If the page was already sent
in precopy then it won't get sent again.

Dave

> 
> > +            void *host = host_from_stream_offset(f, mis, addr, flags, &rb);
> >              if (!host) {
> >                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
> >                  ret = -EINVAL;
> 
> -- 
> 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


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK