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

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Fri, Oct 03, 2014 at 06:47:34PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > 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.

Well it's still completion in the non-postcopy case; if you do think
of a good obvious name then I'd be happy to change it.
(Another way would be to add aparameter to qemu_savevm_state_complete
to make it do one or the other, but some of the conditions in it were
already a bit hairy).

> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  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.

Thanks, fixed.

> > +            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).

Note that I only call save_live_complete once for any one device.
Non-postcopied devices get done in the original loop, postcopied
devices in the new ones, so from the point of view of a device
it's still a complete.

Dave

> 
> -- 
> 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


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK