On 10/05/2023 11:40, Juan Quintela wrote:
External email: Use caution opening links or attachments
Avihai Horon <avih...@nvidia.com> wrote:
Add precopy initial data handshake between source and destination upon
migration setup. The purpose of the handshake is to notify the
destination that precopy initial data is used and which migration users
(i.e., SaveStateEntry) are going to use it.
The handshake is done in two levels. First, a general enable command is
sent to notify the destination migration code that precopy initial data
is used.
Then, for each migration user in the source that supports precopy
initial data, an enable command is sent to its counterpart in the
destination:
If both support it, precopy initial data will be used for them.
If source doesn't support it, precopy initial data will not be used for
them.
If source supports it and destination doesn't, migration will be failed.
To implement it, a new migration command MIG_CMD_INITIAL_DATA_ENABLE is
added, as well as a new SaveVMHandlers handler initial_data_advise.
Calling the handler advises the migration user that precopy initial data
is used and its return value indicates whether precopy initial data is
supported by it.
Signed-off-by: Avihai Horon <avih...@nvidia.com>
diff --git a/migration/savevm.c b/migration/savevm.c
index a9181b444b..2740defdf0 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -71,6 +71,13 @@
const unsigned int postcopy_ram_discard_version;
+typedef struct {
+ uint8_t general_enable;
I miss a comment for this field.
I think that you only use the values 0 and 1
And that 1 means something like: we require this feature and do nothing
And that 0 means that this is a device that uses the feature and
<something, something>
Correct.
But if we assume both source and destination will enable the capability
then general_enable can just be dropped.
+ uint8_t reserved[7];
+ uint8_t idstr[256];
+ uint32_t instance_id;
+} InitialDataInfo;
We have several "reserved" space here. Do we want a Version field?
You mean SaveStateEntry->version_id field?
If so, then I don't think it's necessary, it's checked later on when
device data is sent.
It don't seem that we need a size field, as the command is fixed length.
@@ -90,6 +97,8 @@ enum qemu_vm_cmd {
MIG_CMD_ENABLE_COLO, /* Enable COLO */
MIG_CMD_POSTCOPY_RESUME, /* resume postcopy on dest */
MIG_CMD_RECV_BITMAP, /* Request for recved bitmap on dst */
+
Spurious blank line
Will remove.
+ MIG_CMD_INITIAL_DATA_ENABLE, /* Enable precopy initial data in dest */
MIG_CMD_MAX
+void qemu_savevm_send_initial_data_enable(MigrationState *ms, QEMUFile *f)
+{
+ SaveStateEntry *se;
+ InitialDataInfo buf;
Small nit.
The new way in the block to declare that something needs to be
initialized to zero is:
InitialDataInfo buf = {};
And no, I have no clue if this makes the compiler generate any better code.
Will change.
+ /* Enable precopy initial data generally in the migration */
+ memset(&buf, 0, sizeof(buf));
+ buf.general_enable = 1;
+ qemu_savevm_command_send(f, MIG_CMD_INITIAL_DATA_ENABLE, sizeof(buf),
+ (uint8_t *)&buf);
+ trace_savevm_send_initial_data_enable(buf.general_enable, (char
*)buf.idstr,
+ buf.instance_id);
No buf.idstr here.
Why do we need a command before the loop and seeing if we are having any
device that requires this.
If we assume both source and destination will enable the capability then
this can be dropped as well.
+ /* Enable precopy initial data for each migration user that supports it */
+ QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+ if (!se->ops || !se->ops->initial_data_advise) {
+ continue;
+ }
+
+ if (!se->ops->initial_data_advise(se->opaque)) {
+ continue;
+ }
Is this callback going to send anything?
Nope.
+
+ memset(&buf, 0, sizeof(buf));
+ memcpy(buf.idstr, se->idstr, sizeof(buf.idstr));
+ buf.instance_id = se->instance_id;
+
+ qemu_savevm_command_send(f, MIG_CMD_INITIAL_DATA_ENABLE, sizeof(buf),
+ (uint8_t *)&buf);
+ trace_savevm_send_initial_data_enable(
+ buf.general_enable, (char *)buf.idstr, buf.instance_id);
It is me or general_enable is always zero here?
Yes, it's always 0.
But as I wrote above, if we assume both source and destination will
enable the capability then general_enable can be dropped.
+ }
+}
+
void qemu_savevm_send_colo_enable(QEMUFile *f)
{
trace_savevm_send_colo_enable();
@@ -2314,6 +2359,60 @@ static int
loadvm_process_enable_colo(MigrationIncomingState *mis)
return ret;
}
+static int loadvm_handle_initial_data_enable(MigrationIncomingState *mis)
+{
+ InitialDataInfo buf;
+ SaveStateEntry *se;
+ ssize_t read_size;
+
+ read_size = qemu_get_buffer(mis->from_src_file, (void *)&buf, sizeof(buf));
+ if (read_size != sizeof(buf)) {
+ error_report("%s: Could not get data buffer, read_size %ld, len %lu",
+ __func__, read_size, sizeof(buf));
+
+ return -EIO;
+ }
+
+ /* Enable precopy initial data generally in the migration */
+ if (buf.general_enable) {
+ mis->initial_data_enabled = true;
+ trace_loadvm_handle_initial_data_enable(
+ buf.general_enable, (char *)buf.idstr, buf.instance_id);
+
+ return 0;
+ }
+
+ /* Enable precopy initial data for a specific migration user */
+ se = find_se((char *)buf.idstr, buf.instance_id);
+ if (!se) {
+ error_report("%s: Could not find SaveStateEntry, idstr '%s', "
+ "instance_id %" PRIu32,
+ __func__, buf.idstr, buf.instance_id);
+
+ return -ENOENT;
+ }
+
+ if (!se->ops || !se->ops->initial_data_advise) {
+ error_report("%s: '%s' doesn't have required "
+ "initial_data_advise op",
+ __func__, buf.idstr);
+
+ return -EOPNOTSUPP;
+ }
+
+ if (!se->ops->initial_data_advise(se->opaque)) {
+ error_report("%s: '%s' doesn't support precopy initial data", __func__,
+ buf.idstr);
This is not your fault. Just venting.
And here we are, again, with a place where we can't return errors. Sniff.
+
+ return -EOPNOTSUPP;
+ }
I have to wait until I see the usage later in the series, but it is a
good idea to have a single handle for source and destination, and not
passing at least a parameter telling where are we?
Are you referring here to the initial_data_advise() handler?
If so, then I didn't need to distinguish between source and destination.
But I can add a parameter if you think it could be useful/good practice.
Really nice patch, very good done and very good integrated with the
surrounded style. A pleasure.
Thanks!
