Re: [Qemu-devel] [PATCH v5 18/45] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Wed, Feb 25, 2015 at 04:51:41PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > MIG_CMD_PACKAGED is a migration command that allows a chunk
> > of migration stream to be sent in one go, and be received by
> > a separate instance of the loadvm loop while not interacting
> > with the migration stream.
> 
> Hrm.  I'd be more comfortable if the semantics of CMD_PACKAGED were
> defined in terms of visible effects on the other end, rather than in
> terms of how it's implemented internally.
> 
> > This is used by postcopy to load device state (from the package)
> > while loading memory pages from the main stream.
> 
> Which makes the above paragraph a bit misleading - the whole point
> here is that loading the package data *does* interact with the
> migration stream - just that it's the migration stream after the end
> of the package.

Hmm, how about:


MIG_CMD_PACKAGED is a migration command that wraps a chunk of migration
stream inside a package whose length can be determined purely by reading
its header.  The destination guarantees that the whole MIG_CMD_PACKAGED is
read off the stream prior to parsing the contents.

This is used by postcopy to load device state (from the package)
while leaving the main stream free to receive memory pages.


> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  include/sysemu/sysemu.h |  4 +++
> >  savevm.c                | 82 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  trace-events            |  4 +++
> >  3 files changed, 90 insertions(+)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index d6a6d51..e83bf80 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -87,6 +87,7 @@ enum qemu_vm_cmd {
> >      MIG_CMD_INVALID = 0,       /* Must be 0 */
> >      MIG_CMD_OPEN_RETURN_PATH,  /* Tell the dest to open the Return path */
> >      MIG_CMD_PING,              /* Request a PONG on the RP */
> > +    MIG_CMD_PACKAGED,          /* Send a wrapped stream within this stream 
> > */
> >  
> >      MIG_CMD_POSTCOPY_ADVISE = 20,  /* Prior to any page transfers, just
> >                                        warn we might want to do PC */
> > @@ -101,6 +102,8 @@ enum qemu_vm_cmd {
> >  
> >  };
> >  
> > +#define MAX_VM_CMD_PACKAGED_SIZE (1ul << 24)
> > +
> >  bool qemu_savevm_state_blocked(Error **errp);
> >  void qemu_savevm_state_begin(QEMUFile *f,
> >                               const MigrationParams *params);
> > @@ -113,6 +116,7 @@ void qemu_savevm_command_send(QEMUFile *f, enum 
> > qemu_vm_cmd command,
> >                                uint16_t len, uint8_t *data);
> >  void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
> >  void qemu_savevm_send_open_return_path(QEMUFile *f);
> > +void qemu_savevm_send_packaged(QEMUFile *f, const QEMUSizedBuffer *qsb);
> >  void qemu_savevm_send_postcopy_advise(QEMUFile *f);
> >  void qemu_savevm_send_postcopy_listen(QEMUFile *f);
> >  void qemu_savevm_send_postcopy_run(QEMUFile *f);
> > diff --git a/savevm.c b/savevm.c
> > index e31ccb0..f65bff3 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -636,6 +636,38 @@ void qemu_savevm_send_open_return_path(QEMUFile *f)
> >      qemu_savevm_command_send(f, MIG_CMD_OPEN_RETURN_PATH, 0, NULL);
> >  }
> >  
> > +/* We have a buffer of data to send; we don't want that all to be loaded
> > + * by the command itself, so the command contains just the length of the
> > + * extra buffer that we then send straight after it.
> > + * TODO: Must be a better way to organise that
> > + */
> > +void qemu_savevm_send_packaged(QEMUFile *f, const QEMUSizedBuffer *qsb)
> > +{
> > +    size_t cur_iov;
> > +    size_t len = qsb_get_length(qsb);
> > +    uint32_t tmp;
> > +
> > +    tmp = cpu_to_be32(len);
> > +
> > +    trace_qemu_savevm_send_packaged();
> > +    qemu_savevm_command_send(f, MIG_CMD_PACKAGED, 4, (uint8_t *)&tmp);
> > +
> > +    /* all the data follows (concatinating the iov's) */
> > +    for (cur_iov = 0; cur_iov < qsb->n_iov; cur_iov++) {
> > +        /* The iov entries are partially filled */
> > +        size_t towrite = (qsb->iov[cur_iov].iov_len > len) ?
> > +                              len :
> > +                              qsb->iov[cur_iov].iov_len;
> > +        len -= towrite;
> > +
> > +        if (!towrite) {
> > +            break;
> > +        }
> > +
> > +        qemu_put_buffer(f, qsb->iov[cur_iov].iov_base, towrite);
> > +    }
> > +}
> > +
> >  /* Send prior to any postcopy transfer */
> >  void qemu_savevm_send_postcopy_advise(QEMUFile *f)
> >  {
> > @@ -1265,6 +1297,48 @@ static int 
> > loadvm_process_command_simple_lencheck(const char *name,
> >      return 0;
> >  }
> >  
> > +/* Immediately following this command is a blob of data containing an 
> > embedded
> > + * chunk of migration stream; read it and load it.
> > + */
> > +static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis,
> > +                                      uint32_t length)
> > +{
> > +    int ret;
> > +    uint8_t *buffer;
> > +    QEMUSizedBuffer *qsb;
> > +
> > +    trace_loadvm_handle_cmd_packaged(length);
> > +
> > +    if (length > MAX_VM_CMD_PACKAGED_SIZE) {
> > +        error_report("Unreasonably large packaged state: %u", length);
> > +        return -1;
> 
> It would be a good idea to check this on the send side as well as
> receive, wouldn't it?

Yes, I had been doing that in the postcopy code that called the
send code; but I've now moved it down into savevm_send_packaged.
Thanks.

Dave

> > +    }
> > +    buffer = g_malloc0(length);
> > +    ret = qemu_get_buffer(mis->file, buffer, (int)length);
> > +    if (ret != length) {
> > +        g_free(buffer);
> > +        error_report("CMD_PACKAGED: Buffer receive fail ret=%d 
> > length=%d\n",
> > +                ret, length);
> > +        return (re/t < 0) ? ret : -EAGAIN;
> > +    }
> > +    trace_loadvm_handle_cmd_packaged_received(ret);
> > +
> > +    /* Setup a dummy QEMUFile that actually reads from the buffer */
> > +    qsb = qsb_create(buffer, length);
> > +    g_free(buffer); /* Because qsb_create copies */
> > +    if (!qsb) {
> > +        error_report("Unable to create qsb");
> > +    }
> > +    QEMUFile *packf = qemu_bufopen("r", qsb);
> > +
> > +    ret = qemu_loadvm_state_main(packf, mis);
> > +    trace_loadvm_handle_cmd_packaged_main(ret);
> > +    qemu_fclose(packf);
> > +    qsb_free(qsb);
> > +
> > +    return ret;
> > +}
> > +
> >  /*
> >   * Process an incoming 'QEMU_VM_COMMAND'
> >   * negative return on error (will issue error message)
> > @@ -1315,6 +1389,14 @@ static int loadvm_process_command(QEMUFile *f)
> >          migrate_send_rp_pong(mis, tmp32);
> >          break;
> >  
> > +    case MIG_CMD_PACKAGED:
> > +        if (loadvm_process_command_simple_lencheck("CMD_POSTCOPY_PACKAGED",
> > +            len, 4)) {
> > +            return -1;
> > +         }
> > +        tmp32 = qemu_get_be32(f);
> > +        return loadvm_handle_cmd_packaged(mis, tmp32);
> > +
> >      case MIG_CMD_POSTCOPY_ADVISE:
> >          if (loadvm_process_command_simple_lencheck("CMD_POSTCOPY_ADVISE",
> >                                                     len, 16)) {
> > diff --git a/trace-events b/trace-events
> > index 050f553..cbf995c 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1171,6 +1171,10 @@ qemu_loadvm_state_main(void) ""
> >  qemu_loadvm_state_main_quit_parent(void) ""
> >  qemu_loadvm_state_post_main(int ret) "%d"
> >  qemu_loadvm_state_section_startfull(uint32_t section_id, const char 
> > *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u"
> > +qemu_savevm_send_packaged(void) ""
> > +loadvm_handle_cmd_packaged(unsigned int length) "%u"
> > +loadvm_handle_cmd_packaged_main(int ret) "%d"
> > +loadvm_handle_cmd_packaged_received(int ret) "%d"
> >  loadvm_postcopy_handle_advise(void) ""
> >  loadvm_postcopy_handle_end(void) ""
> >  loadvm_postcopy_handle_listen(void) ""
> 
> -- 
> 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 / address@hidden / Manchester, UK