Re: [Qemu-devel] [PATCH] migration: isolate return path on src

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> There are some places that binded "return path" with postcopy. Let's be
> prepared for its usage even without postcopy. This patch mainly did this
> on source side.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> standalone patch isolated from the return path series. ok to be picked
> up in case one day we'll re-face the return path enablement.
> With it, we are ready on source side. The dst side change has been queued.
> ---
>  migration/migration.c  | 11 ++++++-----
>  migration/trace-events |  4 ++--
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ad29e53..96e549e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s, int current_active_state,
>       * cleaning everything else up (since if there are no failures
>       * it will wait for the destination to send it's status in
>       * a SHUT command).
> -     * Postcopy opens rp if enabled (even if it's not avtivated)
>       */
> -    if (migrate_postcopy_ram()) {
> +    if (s->rp_state.from_dst_file) {
>          int rp_error;
> -        trace_migration_completion_postcopy_end_before_rp();
> +        trace_migration_return_path_end_before();
>          rp_error = await_return_path_close_on_source(s);
> -        trace_migration_completion_postcopy_end_after_rp(rp_error);
> +        trace_migration_return_path_end_after(rp_error);
>          if (rp_error) {
>              goto fail_invalidate;
>          }
> @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque)
>  
>      qemu_savevm_state_header(s->to_dst_file);
>  
> -    if (migrate_postcopy_ram()) {
> +    if (s->to_dst_file) {
>          /* Now tell the dest that it should open its end so it can reply */
>          qemu_savevm_send_open_return_path(s->to_dst_file);
>  
>          /* And do a ping that will make stuff easier to debug */
>          qemu_savevm_send_ping(s->to_dst_file, 1);

I'm confused.
The migration_thread on the source will always have a s->to_dst_file
there so why test?

But also, we can't send the 'open return path' and 'send ping' messages
without a guard; sending them to old QEMUs will cause them to fail when
they don't know what the message is.   I suspect sending them to
slightly-old QEMUs will cause them to fail when they try and send a 
return message if the destination can't send it.

I can see a  if (s->rp_state.from_dst_file) making some sense if it's
done at the right point.

Dave

> +    }
>  
> +    if (migrate_postcopy_ram()) {
>          /*
>           * Tell the destination that we *might* want to do postcopy later;
>           * if the other end can't do postcopy it should fail now, nice and
> diff --git a/migration/trace-events b/migration/trace-events
> index 5b8ccf3..38345be 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -88,8 +88,8 @@ migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
>  migration_completion_file_err(void) ""
>  migration_completion_postcopy_end(void) ""
>  migration_completion_postcopy_end_after_complete(void) ""
> -migration_completion_postcopy_end_before_rp(void) ""
> -migration_completion_postcopy_end_after_rp(int rp_error) "%d"
> +migration_return_path_end_before(void) ""
> +migration_return_path_end_after(int rp_error) "%d"
>  migration_thread_after_loop(void) ""
>  migration_thread_file_err(void) ""
>  migration_thread_setup_complete(void) ""
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK