diff mbox

[v8,25/54] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream

Message ID 1443515898-3594-26-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert Sept. 29, 2015, 8:37 a.m. UTC
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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 <dgilbert@redhat.com>
Reviewed-by: Amit Shah <amit.shah@redhat.com>
---
 include/sysemu/sysemu.h |   4 ++
 migration/savevm.c      | 106 +++++++++++++++++++++++++++++++++++++++++++++---
 trace-events            |   4 ++
 3 files changed, 109 insertions(+), 5 deletions(-)

Comments

Juan Quintela Oct. 20, 2015, 1:25 p.m. UTC | #1
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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 <dgilbert@redhat.com>
> Reviewed-by: Amit Shah <amit.shah@redhat.com>


Reviewed-by: Juan Quintela <quintela@redhat.com>

But I propose the change below


> +    size_t len = qsb_get_length(qsb);

....

> +    /* 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;

Or something have been very wrong here, or qsb->iov[cur_iov].iov_len can
never be > len.  So this should be the same than:

size_t towrite = MIN(qsb->iov[cur_iov].iov_len, len);

right?


> +        len -= towrite;
> +
> +        if (!towrite) {
> +            break;
> +        }

This should never happen, right?  And if we want to be extra safe,


> +    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);

Migration code is re-entrant!!!!!  Who would have guessed O:-)

Later, Juan.
Dr. David Alan Gilbert Oct. 26, 2015, 4:21 p.m. UTC | #2
* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > 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 <dgilbert@redhat.com>
> > Reviewed-by: Amit Shah <amit.shah@redhat.com>
> 
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> But I propose the change below
> 
> 
> > +    size_t len = qsb_get_length(qsb);
> 
> ....
> 
> > +    /* 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;
> 
> Or something have been very wrong here, or qsb->iov[cur_iov].iov_len can
> never be > len.  So this should be the same than:
> 
> size_t towrite = MIN(qsb->iov[cur_iov].iov_len, len);
> 
> right?

Done.

> > +        len -= towrite;
> > +
> > +        if (!towrite) {
> > +            break;
> > +        }
> 
> This should never happen, right?  And if we want to be extra safe,

qsb_get_length() returns the amount of data in the qsb, not the
amount of allocated space; so it's legal for the qsb to have
allocated an iov entry but not actually put any data in it yet.
Will it have done that in our case? I don't think so, but no reason
to make assumptions.

> > +    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);
> 
> Migration code is re-entrant!!!!!  Who would have guessed O:-)

To a very limited degree; there's global state shotgunned around everywhere
(e.g. in the RAM code).

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 204b1c3..9c78d71 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -99,9 +99,12 @@  enum qemu_vm_cmd {
     MIG_CMD_POSTCOPY_RAM_DISCARD,  /* A list of pages to discard that
                                       were previously sent during
                                       precopy but are dirty. */
+    MIG_CMD_PACKAGED,          /* Send a wrapped stream within this stream */
     MIG_CMD_MAX
 };
 
+#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);
@@ -114,6 +117,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);
+int 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/migration/savevm.c b/migration/savevm.c
index 7af8165..de20b95 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -74,6 +74,7 @@  static struct mig_cmd_args {
     [MIG_CMD_POSTCOPY_RUN]     = { .len =  0, .name = "POSTCOPY_RUN" },
     [MIG_CMD_POSTCOPY_RAM_DISCARD] = {
                                    .len = -1, .name = "POSTCOPY_RAM_DISCARD" },
+    [MIG_CMD_PACKAGED]         = { .len =  4, .name = "PACKAGED" },
     [MIG_CMD_MAX]              = { .len = -1, .name = "MAX" },
 };
 
@@ -747,6 +748,50 @@  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
+ *
+ * Returns:
+ *    0 on success
+ *    -ve on error
+ */
+int qemu_savevm_send_packaged(QEMUFile *f, const QEMUSizedBuffer *qsb)
+{
+    size_t cur_iov;
+    size_t len = qsb_get_length(qsb);
+    uint32_t tmp;
+
+    if (len > MAX_VM_CMD_PACKAGED_SIZE) {
+        error_report("%s: Unreasonably large packaged state: %zu",
+                     __func__, len);
+        return -1;
+    }
+
+    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);
+    }
+
+    return 0;
+}
+
 /* Send prior to any postcopy transfer */
 void qemu_savevm_send_postcopy_advise(QEMUFile *f)
 {
@@ -1296,12 +1341,59 @@  static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
 }
 
 /**
- * loadvm_process_command: Process an incoming 'QEMU_VM_COMMAND'
+ * Immediately following this command is a blob of data containing an embedded
+ * chunk of migration stream; read it and load it.
+ *
+ * @mis: Incoming state
+ * @length: Length of packaged data to read
+ *
+ * Returns: Negative values on error
  *
- * Returns: 0 on just a normal return
- *          LOADVM_QUIT All good, but exit the loop
- *          <0 error (in which case it will issue an error message).
- * @f: The stream to read the command data from.
+ */
+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;
+    }
+    buffer = g_malloc0(length);
+    ret = qemu_get_buffer(mis->from_src_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 (ret < 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'
+ * 0           just a normal return
+ * LOADVM_QUIT All good, but exit the loop
+ * <0          Error
  */
 static int loadvm_process_command(QEMUFile *f)
 {
@@ -1351,6 +1443,10 @@  static int loadvm_process_command(QEMUFile *f)
         migrate_send_rp_pong(mis, tmp32);
         break;
 
+    case MIG_CMD_PACKAGED:
+        tmp32 = qemu_get_be32(f);
+        return loadvm_handle_cmd_packaged(mis, tmp32);
+
     case MIG_CMD_POSTCOPY_ADVISE:
         tmp64a = qemu_get_be64(f); /* hps */
         tmp64b = qemu_get_be64(f); /* tps */
diff --git a/trace-events b/trace-events
index cc6668f..4bc05fd 100644
--- a/trace-events
+++ b/trace-events
@@ -1201,6 +1201,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_listen(void) ""
 loadvm_postcopy_handle_run(void) ""