Avihai Horon <avih...@nvidia.com> wrote:
> Add the core functionality of precopy initial data, which allows the
> destination to ACK that initial data has been loaded and the source to
> wait for this ACK before completing the migration.
>
> A new return path command MIG_RP_MSG_INITIAL_DATA_LOADED_ACK is added.
> It is sent by the destination after precopy initial data is loaded to
> ACK to the source that precopy initial data has been loaded.
>
> In addition, two new SaveVMHandlers handlers are added:
> 1. is_initial_data_active which indicates whether precopy initial data
> is used for this migration user (i.e., SaveStateEntry).
> 2. initial_data_loaded which indicates whether precopy initial data has
> been loaded by this migration user.
>
> Signed-off-by: Avihai Horon <avih...@nvidia.com>
> @@ -1401,6 +1412,8 @@ void migrate_init(MigrationState *s)
> s->vm_was_running = false;
> s->iteration_initial_bytes = 0;
> s->threshold_size = 0;
> +
> + s->initial_data_loaded_acked = false;
In general, you let a blank line for the stuff you add, when all the
previous fields don't do that. Can you remove it.
> @@ -2704,6 +2725,20 @@ static void migration_update_counters(MigrationState
> *s,
> bandwidth, s->threshold_size);
> }
>
> +static bool initial_data_loaded_acked(MigrationState *s)
> +{
> + if (!migrate_precopy_initial_data()) {
> + return true;
> + }
> +
> + /* No reason to wait for precopy initial data loaded ACK if VM is
> stopped */
> + if (!runstate_is_running()) {
> + return true;
> + }
Thinking loud here.
What happens if we start a migration. Guest is running.
We enable precopy_initial_data().
And then we stop the guest.
Are we going to receive data that expect this return false? Or it is
handled somewhere else?
> @@ -2719,6 +2754,7 @@ static MigIterateState
> migration_iteration_run(MigrationState *s)
> {
> uint64_t must_precopy, can_postcopy;
> bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
> + bool initial_data_loaded = initial_data_loaded_acked(s);
>
> qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy);
> uint64_t pending_size = must_precopy + can_postcopy;
> @@ -2731,7 +2767,8 @@ static MigIterateState
> migration_iteration_run(MigrationState *s)
> trace_migrate_pending_exact(pending_size, must_precopy,
> can_postcopy);
> }
>
> - if (!pending_size || pending_size < s->threshold_size) {
> + if ((!pending_size || pending_size < s->threshold_size) &&
> + initial_data_loaded) {
> trace_migration_thread_low_pending(pending_size);
> migration_completion(s);
> return MIG_ITERATE_BREAK;
For this specific variable, I think I am going to add something more
general that this can piggy back.
For the migration tests I need exactly this functionality. I want
migration to run until the test decided that it is a good idea to
finish. I.e. For testing xbzrle I need at least three iterations, to
test auto_converge I need a minimum of 13 iterations. And I am going to
do exactly what you have done here.
Will came back to you after I think something.
> @@ -2739,7 +2776,7 @@ static MigIterateState
> migration_iteration_run(MigrationState *s)
>
> /* Still a significant amount to transfer */
> if (!in_postcopy && must_precopy <= s->threshold_size &&
> - qatomic_read(&s->start_postcopy)) {
> + initial_data_loaded && qatomic_read(&s->start_postcopy)) {
> if (postcopy_start(s)) {
> error_report("%s: postcopy failed to start", __func__);
> }
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 2740defdf0..7a94deda3b 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2504,6 +2504,39 @@ static int loadvm_process_command(QEMUFile *f)
> return 0;
> }
>
> +static int qemu_loadvm_initial_data_loaded_ack(MigrationIncomingState *mis)
> +{
> + SaveStateEntry *se;
> + int ret;
> +
> + if (!mis->initial_data_enabled || mis->initial_data_loaded_ack_sent) {
> + return 0;
> + }
> +
> + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> + if (!se->ops || !se->ops->initial_data_loaded) {
> + continue;
> + }
> +
> + if (!se->ops->is_initial_data_active ||
> + !se->ops->is_initial_data_active(se->opaque)) {
> + continue;
> + }
If you don't have any other use for is_initial_data_active() I think you
can integrate the functionality with initial_data_loaded().
If it is not active just return 1?
> +
> + if (!se->ops->initial_data_loaded(se->opaque)) {
> + return 0;
> + }
> + }
> +
> + ret = migrate_send_rp_initial_data_loaded_ack(mis);
> + if (!ret) {
> + mis->initial_data_loaded_ack_sent = true;
> + trace_loadvm_initial_data_loaded_acked();
> + }
> +
> + return ret;
> +}
Later, Juan
