Re: [Qemu-devel] [PATCH v4 3/5] migration: Create load_setup()/cleanup() methods

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

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > > * Juan Quintela (quintela@redhat.com) wrote:
> > >> We need to do things at load time and at cleanup time.
> > >> 
> > >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> > >> 
> > >> --
> > >> 
> > >> Move the printing of the error message so we can print the device
> > >> giving the error.
> > >> Add call to postcopy stuff
> > >> ---
> > >>  include/migration/register.h |  2 ++
> > >>  migration/savevm.c           | 45 +++++++++++++++++++++++++++++++++++++++++++-
> > >>  migration/savevm.h           |  1 +
> > >>  migration/trace-events       |  2 ++
> > >>  4 files changed, 49 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/include/migration/register.h b/include/migration/register.h
> > >> index 938ea2b..a0f1edd 100644
> > >> --- a/include/migration/register.h
> > >> +++ b/include/migration/register.h
> > >> @@ -39,6 +39,8 @@ typedef struct SaveVMHandlers {
> > >>                                uint64_t *non_postcopiable_pending,
> > >>                                uint64_t *postcopiable_pending);
> > >>      LoadStateHandler *load_state;
> > >> +    int (*load_setup)(QEMUFile *f, void *opaque);
> > >> +    int (*load_cleanup)(void *opaque);
> > >>  } SaveVMHandlers;
> > >>  
> > >>  int register_savevm_live(DeviceState *dev,
> > >> diff --git a/migration/savevm.c b/migration/savevm.c
> > >> index fee11c5..fdd15fa 100644
> > >> --- a/migration/savevm.c
> > >> +++ b/migration/savevm.c
> > >> @@ -1541,7 +1541,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > >>       * got a bad migration state).
> > >>       */
> > >>      migration_incoming_state_destroy();
> > >> -
> > >> +    qemu_loadvm_state_cleanup();
> > >
> > > Is that order right? It seems wrong to call the cleanup
> > > code after MIS is destroyed.
> > > (The precopy path seems to call mis_destroy at the end of
> > > process_incoming_migration_bh which is much later).
> > 
> > we can do either way, for now it don't matters.
> > 
> > Once there, it got me thinking that we are doing things in a very
> > "interesting" way on the incoming side:
> > 
> > (postcopy)
> > 
> > postcopy_ram_incoming_cleanup()
> > migration_incoming_state_destroy()
> > qemu_loadvm_state_cleanup()
> > 
> > (Ok, probably it is better to exchange the last two).
> > 
> > But I *think* that we should move the postcopy_ram_incoming_cleanup()
> > inside ram_load_cleanup(), no?
> 
> postcopy_ram_incoming_cleanup shuts down a thread that's shared across
> all RAMBlock's, so I don't think it can all be merged into
> ram_load_cleanup.   You might be able to do the equivalent of the
> cleanup_range function.

Actually that's wrong, we only call ram_load_cleanup once - because
RAM is special and is only register_savevm_live once, not per device.

So yes you probably can do that.

Dave

> > And we don't have a postcopy_ram_incoming_setup() We could put there the
> > mmap of mis->postcopy_tmp_zero_page and mis->largest_page_size, no?
> 
> Again that's a single shared zero page, not per RAMBlock.
> 
> > I am trying to understand if the postcopy_ram_incoming_init() can be
> > moved soon, but I think no.
> 
> Dave
> 
> > 
> > Later, Juan.
> > 
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK