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

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> wrote:
> > * Juan Quintela (address@hidden) wrote:
> >> We need to do things at load time and at cleanup time.
> >> 
> >> Signed-off-by: Juan Quintela <address@hidden>
> >> 
> >> --
> >> 
> >> 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.

> 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 / address@hidden / Manchester, UK