Re: [Qemu-devel] [PATCH v7 26/42] postcopy: Incoming initialisation

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

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  include/migration/migration.h    |   3 +
> >  include/migration/postcopy-ram.h |  12 ++++
> >  migration/postcopy-ram.c         | 116 +++++++++++++++++++++++++++++++++++++++
> >  migration/ram.c                  |  11 ++++
> >  migration/savevm.c               |   4 ++
> >  trace-events                     |   2 +
> >  6 files changed, 148 insertions(+)
> >
> 
> qemu_hugepage_enable(host_addr, length)?
> 
> > +#ifdef MADV_NOHUGEPAGE
> > +    if (madvise(host_addr, length, MADV_NOHUGEPAGE)) {
> > +        error_report("%s: NOHUGEPAGE: %s", __func__, strerror(errno));
> > +        return -1;
> > +    }
> > +#endif
> 
> qemu_hugepage_disable(host_addr, length)?

I've flipped those both to use the qemu_madvise which is how
it's done in most other places in the codebase (I added
the QEMU_MADV_NOHUGEPAGE since HUGEPAGE was there but not the opposite).

> > +#ifdef MADV_HUGEPAGE
> > +    if (madvise(host_addr, length, MADV_HUGEPAGE)) {
> > +        error_report("%s HUGEPAGE: %s", __func__, strerror(errno));
> > +        return -1;
> > +    }
> > +#endif
> > +
> > +    /*
> > +     * We can also turn off userfault now since we should have all the
> > +     * pages.   It can be useful to leave it on to debug postcopy
> > +     * if you're not sure it's always getting every page.
> > +     */
> 
> qemu_userfault_unregister(host_addr, length)?

Is it worth wrapping that ioctl, when it's already in a function
that has to do other calls around it?

> > +    range_struct.start = (uintptr_t)host_addr;
> > +    range_struct.len = length;
> > +
> > +    if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
> > +        error_report("%s: userfault unregister %s", __func__, strerror(errno));
> > +
> > +        return -1;
> > +    }
> 
> >  
> > +/*
> > + * Allocate data structures etc needed by incoming migration with postcopy-ram
> > + * postcopy-ram's similarly names postcopy_ram_incoming_init does the work
> > + */
> > +int ram_postcopy_incoming_init(MigrationIncomingState *mis)
> > +{
> > +    size_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> > +
> > +    return postcopy_ram_incoming_init(mis, ram_pages);
> > +}
> > +
> 
> ram_postocpy_incoming_init()
> and
> postcopy_ram_incoming_init()
> 
> ouch  Thinking about better names ....

Agreed; suggestions welcome. the 'ram' and 'postcopy_ram' are
both the convention based on the filename.

> >  static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >  {
> >      int flags = 0, ret = 0;
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index e6398dd..f4de52d 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1238,6 +1238,10 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> >          return -1;
> >      }
> >  
> > +    if (ram_postcopy_incoming_init(mis)) {
> > +        return -1;
> > +    }
> > +
> 
> how/where we know that this is called soon enough?

It's driven by the sequence of commands byte that walk it through
the state machine.

> >      postcopy_state_set(mis, POSTCOPY_INCOMING_ADVISE);
> >  
> >      return 0;
> > diff --git a/trace-events b/trace-events
> > index 5e8a120..2ffc1c6 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1498,7 +1498,9 @@ rdma_start_outgoing_migration_after_rdma_source_init(void) ""
> >  
> >  # migration/postcopy-ram.c
> >  postcopy_discard_send_finish(const char *ramblock, int nwords, int ncmds) "%s mask words sent=%d in %d commands"
> > +postcopy_cleanup_area(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=%zx length=%zx"
> >  postcopy_ram_discard_range(void *start, void *end) "%p,%p"
> > +postcopy_init_area(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=%zx length=%zx"
> 
> once here, if we have range names before, what about:
> 
> postcopy_ram_cleanup_range()
> postcopy_ram_init_range()

Done.

> And let the ram* functions the same?

Not sure which ones those refer to?

Dave

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