Re: [Qemu-devel] [PATCH 03/17] migration: split common postcopy out of ram postcopy

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

24.01.2017 22:53, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> 24.01.2017 12:24, Juan Quintela wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>> Split common postcopy staff from ram postcopy staff.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    include/migration/migration.h |  1 +
>>>>    migration/migration.c         | 39 +++++++++++++++++++++++++++------------
>>>>    migration/postcopy-ram.c      |  4 +++-
>>>>    migration/savevm.c            | 31 ++++++++++++++++++++++---------
>>>>    4 files changed, 53 insertions(+), 22 deletions(-)
>>>>
>>> Hi
>>>
>>> {
>>>>        MigrationState *s;
>>>> @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
>>>>         * need to tell the destination to throw any pages it's already received
>>>>         * that are dirty
>>>>         */
>>>> -    if (ram_postcopy_send_discard_bitmap(ms)) {
>>>> -        error_report("postcopy send discard bitmap failed");
>>>> -        goto fail;
>>>> +    if (migrate_postcopy_ram()) {
>>>> +        if (ram_postcopy_send_discard_bitmap(ms)) {
>>>> +            error_report("postcopy send discard bitmap failed");
>>>> +            goto fail;
>>>> +        }
>>> I will have preffered that for the ram commands, to embed the
>>> migrate_postocpy_ram() check inside them, but that is taste, and
>>> everyone has its own O:-)
>>>
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index e436cb2..c8a71c8 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -73,7 +73,7 @@ static struct mig_cmd_args {
>>>>        [MIG_CMD_INVALID]          = { .len = -1, .name = "INVALID" },
>>>>        [MIG_CMD_OPEN_RETURN_PATH] = { .len =  0, .name = "OPEN_RETURN_PATH" },
>>>>        [MIG_CMD_PING]             = { .len = sizeof(uint32_t), .name = "PING" },
>>>> -    [MIG_CMD_POSTCOPY_ADVISE]  = { .len = 16, .name = "POSTCOPY_ADVISE" },
>>>> +    [MIG_CMD_POSTCOPY_ADVISE]  = { .len = -1, .name = "POSTCOPY_ADVISE" },
>>>>        [MIG_CMD_POSTCOPY_LISTEN]  = { .len =  0, .name = "POSTCOPY_LISTEN" },
>>>>        [MIG_CMD_POSTCOPY_RUN]     = { .len =  0, .name = "POSTCOPY_RUN" },
>>>>        [MIG_CMD_POSTCOPY_RAM_DISCARD] = {
>>>> @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len)
>>>>    /* Send prior to any postcopy transfer */
>>>>    void qemu_savevm_send_postcopy_advise(QEMUFile *f)
>>>>    {
>>>> -    uint64_t tmp[2];
>>>> -    tmp[0] = cpu_to_be64(getpagesize());
>>>> -    tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits());
>>>> +    if (migrate_postcopy_ram()) {
>>>> +        uint64_t tmp[2];
>>>> +        tmp[0] = cpu_to_be64(getpagesize());
>>>> +        tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits());
>>>> -    trace_qemu_savevm_send_postcopy_advise();
>>>> -    qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t *)tmp);
>>>> +        trace_qemu_savevm_send_postcopy_advise();
>>>> +        qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE,
>>>> +                                 16, (uint8_t *)tmp);
>>>> +    } else {
>>>> +        qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL);
>>>> +    }
>>>>    }
>>>>    /* Sent prior to starting the destination running in postcopy, discard pages
>>> I haven't yet figured out why you are reusing this command with a
>>> different number of parameters.
>>> For this to pass, I need that Dave comment on this.
>>>
>>> So,
>>>
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>
>>> conditioned that Dave agrees with this.
>> These parameters are unrelated if ram postcopy is disabled. So, I should be
>> better to have a possibility of skipping them, then to send unneeded numbers
>> and write separate code to read them from the stream (rewriting
>> loadvm_postcopy_handle_advise to just read these two numbers and do nothing
>> about ram postcopy for this case).
> I think I'd prefer either a new command or keeping these fields (probably all 0 ?)
> my worry is what happens in the case if we add a 3rd postcopy subfeature;
> In your case we have three possibilities:
>
>      a) Postcopy RAM only - 16 bytes
>      b) Postcopy persistent-dirty-bitmap only - 0 bytes
>      c) Both - 16 bytes
>
> Lets say we added postcopy-foo in the future and it wanted to add
> another 16 bytes, what would it send if it was foo+persistent-dirty-bitmap and no RAM?
> We'd end up with 16 bytes sent but you'd have to be very careful
> never to get that confused with case (a) above.
>
> (I don't feel too strongly about it though)

Hmm.. Actually I use migrate_postcopy_ram() to distinct these thing in 
loadvm_postcopy_handle_advise.. And it seem like it is a mistake. Are 
migration capabilities available on target?.. On the other hand 
postcopy-test doesn't fail...

>
> Dave
>
>> -- 
>> Best regards,
>> Vladimir
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


-- 
Best regards,
Vladimir