Re: [Qemu-devel] [PATCH v4 39/47] postcopy_ram.c: place_page and helpers

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Fri, Oct 03, 2014 at 06:47:45PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > postcopy_place_page (etc) provide a way for postcopy to place a page
> > into guests memory atomically (using the new remap_anon_pages syscall).
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  include/migration/migration.h    |   2 +
> >  include/migration/postcopy-ram.h |  23 +++++++
> >  postcopy-ram.c                   | 145 
> > ++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 168 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 5bc01d5..58ac7bf 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -96,6 +96,8 @@ struct MigrationIncomingState {
> >      QEMUFile *return_path;
> >      QemuMutex      rp_mutex;    /* We send replies from multiple threads */
> >      PostcopyPMI    postcopy_pmi;
> > +    void          *postcopy_tmp_page;
> > +    long           postcopy_place_skipped; /* Check for incorrect place 
> > ops */
> >  };
> >  
> >  MigrationIncomingState *migration_incoming_get_current(void);
> > diff --git a/include/migration/postcopy-ram.h 
> > b/include/migration/postcopy-ram.h
> > index 413b670..0210491 100644
> > --- a/include/migration/postcopy-ram.h
> > +++ b/include/migration/postcopy-ram.h
> > @@ -80,4 +80,27 @@ void postcopy_discard_send_chunk(MigrationState *ms, 
> > PostcopyDiscardState *pds,
> >  void postcopy_discard_send_finish(MigrationState *ms,
> >                                    PostcopyDiscardState *pds);
> >  
> > +/*
> > + * Place a zero'd page of memory at *host
> > + * returns 0 on success
> > + */
> > +int postcopy_place_zero_page(MigrationIncomingState *mis, void *host,
> > +                             long bitmap_offset);
> > +
> > +/*
> > + * Place a page (from) at (host) efficiently
> > + *    There are restrictions on how 'from' must be mapped, in general best
> > + *    to use other postcopy_ routines to allocate.
> > + * returns 0 on success
> > + */
> > +int postcopy_place_page(MigrationIncomingState *mis, void *host, void 
> > *from,
> > +                        long bitmap_offset);
> > +
> > +/*
> > + * Allocate a page of memory that can be mapped at a later point in time
> > + * using postcopy_place_page
> > + * Returns: Pointer to allocated page
> > + */
> > +void *postcopy_get_tmp_page(MigrationIncomingState *mis, long 
> > bitmap_offset);
> > +
> >  #endif
> > diff --git a/postcopy-ram.c b/postcopy-ram.c
> > index 8b2a035..19d4b20 100644
> > --- a/postcopy-ram.c
> > +++ b/postcopy-ram.c
> > @@ -229,7 +229,6 @@ static PostcopyPMIState postcopy_pmi_get_state_nolock(
> >  }
> >  
> >  /* Retrieve the state of the given page */
> > -__attribute__ (( unused )) /* Until later in patch series */
> >  static PostcopyPMIState postcopy_pmi_get_state(MigrationIncomingState *mis,
> >                                                 size_t bitmap_index)
> >  {
> > @@ -245,7 +244,6 @@ static PostcopyPMIState 
> > postcopy_pmi_get_state(MigrationIncomingState *mis,
> >   * Set the page state to the given state if the previous state was as 
> > expected
> >   * Return the actual previous state.
> >   */
> > -__attribute__ (( unused )) /* Until later in patch series */
> >  static PostcopyPMIState postcopy_pmi_change_state(MigrationIncomingState 
> > *mis,
> >                                             size_t bitmap_index,
> >                                             PostcopyPMIState expected_state,
> > @@ -464,6 +462,7 @@ static int cleanup_area(const char *block_name, void 
> > *host_addr,
> >  int postcopy_ram_incoming_init(MigrationIncomingState *mis, size_t 
> > ram_pages)
> >  {
> >      postcopy_pmi_init(mis, ram_pages);
> > +    mis->postcopy_place_skipped = -1;
> >  
> >      if (qemu_ram_foreach_block(init_area, mis)) {
> >          return -1;
> > @@ -482,6 +481,10 @@ int 
> > postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> >          return -1;
> >      }
> >  
> > +    if (mis->postcopy_tmp_page) {
> > +        munmap(mis->postcopy_tmp_page, getpagesize());
> > +        mis->postcopy_tmp_page = NULL;
> > +    }
> >      return 0;
> >  }
> >  
> > @@ -551,6 +554,126 @@ int postcopy_ram_enable_notify(MigrationIncomingState 
> > *mis)
> >      return 0;
> >  }
> >  
> > +/*
> > + * Place a zero'd page of memory at *host
> > + * returns 0 on success
> > + * bitmap_offset: Index into the migration bitmaps
> > + */
> > +int postcopy_place_zero_page(MigrationIncomingState *mis, void *host,
> > +                             long bitmap_offset)
> > +{
> > +    void *tmp = postcopy_get_tmp_page(mis, bitmap_offset);
> > +    if (!tmp) {
> > +        return -ENOMEM;
> > +    }
> > +    *(char *)tmp = 0;
> > +    return postcopy_place_page(mis, host, tmp, bitmap_offset);
> > +}
> > +
> > +/*
> > + * Place a target page (from) at (host) efficiently
> > + *    There are restrictions on how 'from' must be mapped, in general best
> > + *    to use other postcopy_ routines to allocate.
> > + * returns 0 on success
> > + * bitmap_offset: Index into the migration bitmaps
> > + *
> > + * Where HPS > TPS it holds off doing the place until the last TP in the HP
> > + *  and assumes (from, host) point to the last TP in a continuous HP
> > + */
> > +int postcopy_place_page(MigrationIncomingState *mis, void *host, void 
> > *from,
> > +                        long bitmap_offset)
> > +{
> > +    PostcopyPMIState old_state, tmp_state;
> > +    size_t hps = sysconf(_SC_PAGESIZE);
> > +
> > +    /* Only place the page when the last target page within the hp arrives 
> > */
> > +    if ((bitmap_offset + 1) & (mis->postcopy_pmi.host_bits - 1)) {
> > +        DPRINTF("%s: Skipping incomplete hp host=%p from=%p 
> > bitmap_offset=%lx",
> > +                __func__, host, from, bitmap_offset);
> > +        mis->postcopy_place_skipped = bitmap_offset;
> > +        return 0;
> > +    }
> > +
> > +    /*
> > +     * If we skip a page (above) we should end up placing that page before
> > +     * doing anything with other host pages.
> > +     */
> > +    if (mis->postcopy_place_skipped != -1) {
> > +        assert((bitmap_offset & ~(mis->postcopy_pmi.host_bits - 1)) ==
> > +               (mis->postcopy_place_skipped &
> > +                ~(mis->postcopy_pmi.host_bits - 1)));
> > +    }
> > +    mis->postcopy_place_skipped = -1;
> 
> All the above logic seems like you're making assumptions about exactly
> how this function will be invoked which are fragile and a layering
> violation.
> 
> It seems like these lower level functions should work only in host
> pages and have the target->host page consolidation up in the protocol
> handling layer.  Better yet would be to build it into the protocol
> itself that reuqests made by the desination (in destination host page
> chunks) should be answered by the source as a unit, to avoid the
> hassle of splitting and recombining host pages.

I've reworked this for place_page to rebalance it towards the
callers of this code in ram_load; the sending side ensures that
it meets these requirements when in postcopy mode.


<snip>

> > +
> > +/*
> > + * Returns a target page of memory that can be mapped at a later point in 
> > time
> > + * using postcopy_place_page
> > + * The same address is used repeatedly, postcopy_place_page just takes the
> > + * backing page away.
> 
> The same address might be re-used, but I don't see anything that
> actually makes that happen.

The virtual allocation is never freed, and so the address returned here is 
always
the same once initially allocated.  The physical page churns around though
as the one used is put into place.

Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK