Re: [Qemu-devel] possible ahci/migrate fix

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> Hi Alex,
>   Can you see if the attached patch fixes the ahci/migrate failure you
> see;  it won't fail for me however mean I am to it.
>
>

....

>  void migration_object_finalize(void)
>  {
> +    /*
> +     * Cancel the current migration - that will (eventually)
> +     * stop the migration using this structure
> +     */
> +    migrate_fd_cancel(current_migration);

This can only happen during "civilized" exit of qemu, right?
Otherwise, we are changing the migration status.

>      object_unref(OBJECT(current_migration));
>  }
>  
> @@ -3134,6 +3140,7 @@ static void *migration_thread(void *opaque)
>  
>      rcu_register_thread();
>  
> +    object_ref(OBJECT(s));

It is weird that this is not enough :-(

>      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..c1920165f3 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_object_finalize();
> +
>      /* 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;

Ok, it was happening really late.

Once that you are at this, can we rename it?

migration_cleanup()

looks more consistent with everything else, and makes sure that it does
"more" than finalize the object, no?

Furthermore, it is enough to "just" cancel the migration?  It is not
needed that we wait for the migration thread to finish?

If the problem was that migration_thread() was accessing the object
after main thread freed it, then just the ref counting should be enough,
no?  My understanding is that returning from main is the quivalent of
exit() and kill all threads?  Or are we doing something special for that
not to happen?

Later, Juan.