Re: [Qemu-devel] [PATCH] migration: Cleanup during exit

["Alex Bennée" <alex.bennee@linaro.org>]

Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Currently we cleanup the migration object as we exit main after the
> main_loop finishes; however if there's a migration running things
> get messy and we can end up with the migration thread still trying
> to access freed structures.
>
> We now take a ref to the object around the migration thread itself,
> so the act of dropping the ref during exit doesn't cause us to lose
> the state until the thread quits.
>
> Cancelling the migration during migration also tries to get the thread
> to quit.
>
> We do this a bit earlier; so hopefully migration gets out of the way
> before all the devices etc are freed.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/migration/misc.h |  2 +-
>  migration/migration.c    | 10 +++++++++-
>  vl.c                     |  7 ++++++-
>  3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 0471e04d1f..6f9df74436 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -36,7 +36,7 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>
>  /* migration/migration.c */
>  void migration_object_init(void);
> -void migration_object_finalize(void);
> +void migration_shutdown(void);
>  void qemu_start_incoming_migration(const char *uri, Error **errp);
>  bool migration_is_idle(void);
>  void add_migration_state_change_notifier(Notifier *notify);
> diff --git a/migration/migration.c b/migration/migration.c
> index e44f77af02..d45561f9b8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -126,6 +126,7 @@ static bool migration_object_check(MigrationState *ms, Error **errp);
>  static int migration_maybe_pause(MigrationState *s,
>                                   int *current_active_state,
>                                   int new_state);
> +static void migrate_fd_cancel(MigrationState *s);
>
>  void migration_object_init(void)
>  {
> @@ -167,8 +168,13 @@ void migration_object_init(void)
>      }
>  }
>
> -void migration_object_finalize(void)
> +void migration_shutdown(void)
>  {
> +    /*
> +     * Cancel the current migration - that will (eventually)
> +     * stop the migration using this structure
> +     */
> +    migrate_fd_cancel(current_migration);
>      object_unref(OBJECT(current_migration));
>  }
>
> @@ -3134,6 +3140,7 @@ static void *migration_thread(void *opaque)
>
>      rcu_register_thread();
>
> +    object_ref(OBJECT(s));
>      s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>
>      qemu_savevm_state_header(s->to_dst_file);
> @@ -3230,6 +3237,7 @@ static void *migration_thread(void *opaque)
>
>      trace_migration_thread_after_loop();
>      migration_iteration_finish(s);
> +    object_unref(OBJECT(s));
>      rcu_unregister_thread();
>      return NULL;
>  }
> diff --git a/vl.c b/vl.c
> index 2f340686a7..e7f460a114 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4579,6 +4579,12 @@ int main(int argc, char **argv, char **envp)
>
>      gdbserver_cleanup();
>
> +    /*
> +     * cleaning up the migration object cancels any existing migration
> +     * try to do this early so that it also stops using devices.
> +     */
> +    migration_shutdown();
> +

We do like to vary our language over here, I guess shutdown and cleanup
imply subtly different things. FWIW I suspect the gdbserver_cleanup()
should be a shutdown ;-)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>      /* No more vcpu or device emulation activity beyond this point */
>      vm_shutdown();
>
> @@ -4594,7 +4600,6 @@ int main(int argc, char **argv, char **envp)
>      monitor_cleanup();
>      qemu_chr_cleanup();
>      user_creatable_cleanup();
> -    migration_object_finalize();
>      /* TODO: unref root container, check all devices are ok */
>
>      return 0;


--
Alex Bennée