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

[Juan Quintela <quintela@redhat.com>]

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

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?

I am trying to understand if the postcopy_ram_incoming_init() can be
moved soon, but I think no.

Later, Juan.