Re: [Qemu-devel] [PATCH v4 21/47] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages.

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

* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Fri, Oct 03, 2014 at 06:47:27PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Add state variable showing current incoming postcopy state.
> 
> This appears to implement a lot more than just adding a state variable...

It's clearer with the title line;  I've reworded it to:

    Add wrappers and handlers for sending/receiving the postcopy-ram migration messages.
    
    The state of the postcopy process is managed via a series of messages;
       * Add wrappers and handlers for sending/receiving these messages
       * Add state variable that track the current state of postcopy

> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/migration/migration.h |   8 +
> >  include/sysemu/sysemu.h       |  20 +++
> >  savevm.c                      | 335 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 363 insertions(+)
> > 
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 0d9f62d..2c078c4 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -61,6 +61,14 @@ typedef struct MigrationState MigrationState;
> >  struct MigrationIncomingState {
> >      QEMUFile *file;
> >  
> > +    volatile enum {
> 
> What's the reason for the volatile?  I think that really needs a comment.

I've removed it and replaced it by atomic_ functions to access it;
the state is accessed from multiple threads.

> > +        POSTCOPY_RAM_INCOMING_NONE = 0,  /* Initial state - no postcopy */
> > +        POSTCOPY_RAM_INCOMING_ADVISE,
> > +        POSTCOPY_RAM_INCOMING_LISTENING,
> > +        POSTCOPY_RAM_INCOMING_RUNNING,
> > +        POSTCOPY_RAM_INCOMING_END
> > +    } postcopy_ram_state;
> > +
> >      QEMUFile *return_path;
> >      QemuMutex      rp_mutex;    /* We send replies from multiple threads */
> >  };

<snip>

> > diff --git a/savevm.c b/savevm.c
> > index 7236232..b942e8c 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -39,6 +39,7 @@
> >  #include "exec/memory.h"
> >  #include "qmp-commands.h"
> >  #include "trace.h"
> > +#include "qemu/bitops.h"
> >  #include "qemu/iov.h"
> >  #include "block/snapshot.h"
> >  #include "block/qapi.h"
> > @@ -624,6 +625,92 @@ void qemu_savevm_send_openrp(QEMUFile *f)
> >  {
> >      qemu_savevm_command_send(f, QEMU_VM_CMD_OPENRP, 0, NULL);
> >  }
> > +
> > +/* Send prior to any RAM transfer */
> > +void qemu_savevm_send_postcopy_ram_advise(QEMUFile *f)
> > +{
> > +    DPRINTF("send postcopy-ram-advise");
> > +    uint64_t tmp[2];
> > +    tmp[0] = cpu_to_be64(sysconf(_SC_PAGESIZE));
> > +    tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits());
> > +
> > +    qemu_savevm_command_send(f, QEMU_VM_CMD_POSTCOPY_RAM_ADVISE, 16,
> > +                             (uint8_t *)tmp);
> > +}
> > +
> > +/* Prior to running, to cause pages that have been dirtied after precopy
> > + * started to be discarded on the destination.
> > + * CMD_POSTCOPY_RAM_DISCARD consist of:
> > + *  3 byte header (filled in by qemu_savevm_send_postcopy_ram_discard)
> > + *      byte   version (0)
> > + *      byte   offset into the 1st data word containing 1st page of RAMBlock
> 
> I'm not able to follow what that description means.

I've reworded it as:
 *      byte   offset to be subtracted from each page address to deal with
 *             RAMBlocks that don't start on a mask word boundary.

(I've tried reworking this protocol a few times, the offset still seems
to work out easiest, otherwise you end up having to treat the address entries
as potentially signed).

<snip>

> > +    /*
> > +     * Postcopy will be sending lots of small messages along the return path
> > +     * that it needs quick answers to.
> > +     */
> > +    socket_set_nodelay(qemu_get_fd(mis->return_path));
> 
> So, here you break the QEMUFile abstraction and assume you have a
> socket.

Ah yes; I put that in to see if it would help.  Hmm; I've taken it out for now,
I need to see if there's a sensible way to add the abstration.

> > +    while (len) {
> > +        uint64_t startaddr;
> > +        uint32_t mask;
> > +        /*
> > +         * We now have pairs of address, mask
> > +         *   The mask is 32 bits of bitmask starting at 'startaddr'-offset
> > +         *   RAMBlock; e.g. if the RAMBlock started at 8k where TPS=4k
> > +         *   then first_bit_offset=2 and the 1st 2 bits of the mask
> > +         *   aren't relevant to this RAMBlock, and bit 2 corresponds
> > +         *   to the 1st page of this RAMBlock
> 
> Um.. yeah.. can't make much snse of this comment either.

Well, at least it's the one that corresponds to the comment above you couldn't
understand either.
I've reworded it to:
         * We now have pairs of address, mask
         *   Each word of mask is 32 bits, where each bit corresponds to one
         *   target page.
         *   RAMBlocks don't necessarily start on word boundaries, 
         *   and the offset in the header indicates the offset into the 1st
         *   mask word that corresponds to the 1st page of the RAMBlock.

<snip>

> > +/* After this message we must be able to immediately receive page data */
> 
> The purpose of the listen message from a protocol point of view isn't
> really clear to me.  I understand why the destination needs to set up
> the postcopy handling before processing the device data, but why does
> it need an assertion from the source to start this, rather than just
> an internal detail on the load path.

The device migration format isn't structured well enough to allow the
receiver to know the size of the device data without parsing it all.
The parsing isn't structured, so there's no way to do that without
calling all the device code that may read from guest memory.
We solve that by sending all the device data in the CMD_PACKAGED.

While we could assume that a CMD_PACKAGED is always for postcopy data,
instead I don't special case the processing at all of the data inside
the package, keeping that general.  Then the listen message is used
inside that package to start the listener thread off and set up the other
associated state.

Dave

> 
> -- 
> 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