Re: [Qemu-devel] [PATCH v4 28/47] qemu_savevm_state_complete: Postcopy changes

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3589 bytes --]

On Fri, Oct 03, 2014 at 06:47:34PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> When postcopy calls qemu_savevm_state_complete it's not really
> the end of migration, so skip:
>    a) Finishing postcopiable iterative devices - they'll carry on
>    b) The termination byte on the end of the stream.
> 
> We then also add:
>   qemu_savevm_state_postcopy_complete
> which is called at the end of a postcopy migration to call the
> complete methods on devices skipped in the _complete call.

So, we should probably rename qemu_savevm_state_complete() to reflect
the fact that it's no longer actually a completion, but just the
transition from pre-copy to post-copy phases.  A good, brief name
doesn't immediately occur to me, unfortunately.

> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/sysemu/sysemu.h |  1 +
>  savevm.c                | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index e7ff3d0..46665ce 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -113,6 +113,7 @@ void qemu_savevm_state_cancel(void);
>  void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
>                                 uint64_t *res_non_postcopiable,
>                                 uint64_t *res_postcopiable);
> +void qemu_savevm_state_postcopy_complete(QEMUFile *f);
>  void qemu_savevm_command_send(QEMUFile *f, enum qemu_vm_cmd command,
>                                uint16_t len, uint8_t *data);
>  void qemu_savevm_send_reqack(QEMUFile *f, uint32_t value);
> diff --git a/savevm.c b/savevm.c
> index a0cb88b..7c4541d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -854,10 +854,51 @@ int qemu_savevm_state_iterate(QEMUFile *f)
>      return ret;
>  }
>  
> +/*
> + * Calls the complete routines just for those devices that are postcopiable;
> + * causing the last few pages to be sent immediately and doing any associated
> + * cleanup.
> + * Note postcopy also calls the plain qemu_savevm_state_complete to complete
> + * all the other devices, but that happens at the point we switch to postcopy.
> + */
> +void qemu_savevm_state_postcopy_complete(QEMUFile *f)
> +{
> +    SaveStateEntry *se;
> +    int ret;
> +
> +    QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> +        if (!se->ops || !se->ops->save_live_complete ||
> +            !se->ops->can_postcopy) {

So, you check for the presence of a can_postcopy callback, but you
don't ever actually invoke it.

> +            continue;
> +        }
> +        if (se->ops && se->ops->is_active) {
> +            if (!se->ops->is_active(se->opaque)) {
> +                continue;
> +            }
> +        }
> +        trace_savevm_section_start(se->idstr, se->section_id);
> +        /* Section type */
> +        qemu_put_byte(f, QEMU_VM_SECTION_END);
> +        qemu_put_be32(f, se->section_id);
> +
> +        ret = se->ops->save_live_complete(f, se->opaque);

I'm wondering if it might be clearer not to overload the
save_live_complete hook, but instead allow both "execution transition"
(old complete) and "final complete" (postcopy complete) hooks
(expecting only one to be non-NULL in most cases).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]