Re: [PATCH v3 01/17] migration: Remove res_compatible parameter

[Avihai Horon <avihaih@nvidia.com>]

On 23/11/2022 20:23, Dr. David Alan Gilbert wrote:
> External email: Use caution opening links or attachments
>
>
> * Avihai Horon (avihaih@nvidia.com) wrote:
>> On 08/11/2022 19:52, Vladimir Sementsov-Ogievskiy wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 11/3/22 19:16, Avihai Horon wrote:
>>>> From: Juan Quintela <quintela@redhat.com>
>>>>
>>>> It was only used for RAM, and in that case, it means that this amount
>>>> of data was sent for memory.
>>> Not clear for me, what means "this amount of data was sent for
>>> memory"... That amount of data was not yet sent, actually.
>>>
>> Yes, this should be changed to something like:
>>
>> "It was only used for RAM, and in that case, it means that this amount
>> of data still needs to be sent for memory, and can be sent in any phase
>> of migration. The same functionality can be achieved without res_compatible,
>> so just delete the field in all callers and change the definition of
>> res_postcopy accordingly.".
> Sorry, I recently sent a similar comment in reply to Juan's original
> post.
> If I understand correctly though, the dirty bitmap code relies on
> 'postcopy' here to be data only sent during postcopy.

Looks like this patch requires some further discussion.
Since it's not mandatory for this series and I don't want it to block 
this series, I can drop it and some variant of it can be added later on.

Thanks all for the effort!

> Dave
>
>>>> Just delete the field in all callers.
>>>>
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>>    hw/s390x/s390-stattrib.c       |  6 ++----
>>>>    hw/vfio/migration.c            | 10 ++++------
>>>>    hw/vfio/trace-events           |  2 +-
>>>>    include/migration/register.h   | 20 ++++++++++----------
>>>>    migration/block-dirty-bitmap.c |  7 +++----
>>>>    migration/block.c              |  7 +++----
>>>>    migration/migration.c          |  9 ++++-----
>>>>    migration/ram.c                |  8 +++-----
>>>>    migration/savevm.c             | 14 +++++---------
>>>>    migration/savevm.h             |  4 +---
>>>>    migration/trace-events         |  2 +-
>>>>    11 files changed, 37 insertions(+), 52 deletions(-)
>>>>
>>> [..]
>>>
>>>> diff --git a/include/migration/register.h b/include/migration/register.h
>>>> index c1dcff0f90..1950fee6a8 100644
>>>> --- a/include/migration/register.h
>>>> +++ b/include/migration/register.h
>>>> @@ -48,18 +48,18 @@ typedef struct SaveVMHandlers {
>>>>        int (*save_setup)(QEMUFile *f, void *opaque);
>>>>        void (*save_live_pending)(QEMUFile *f, void *opaque,
>>>>                                  uint64_t threshold_size,
>>>> -                              uint64_t *res_precopy_only,
>>>> -                              uint64_t *res_compatible,
>>>> -                              uint64_t *res_postcopy_only);
>>>> +                              uint64_t *rest_precopy,
>>>> +                              uint64_t *rest_postcopy);
>>>>        /* Note for save_live_pending:
>>>> -     * - res_precopy_only is for data which must be migrated in
>>>> precopy phase
>>>> -     *     or in stopped state, in other words - before target vm start
>>>> -     * - res_compatible is for data which may be migrated in any phase
>>>> -     * - res_postcopy_only is for data which must be migrated in
>>>> postcopy phase
>>>> -     *     or in stopped state, in other words - after source vm stop
>>>> +     * - res_precopy is for data which must be migrated in precopy
>>>> +     *     phase or in stopped state, in other words - before target
>>>> +     *     vm start
>>>> +     * - res_postcopy is for data which must be migrated in postcopy
>>>> +     *     phase or in stopped state, in other words - after source vm
>>>> +     *     stop
>>>>         *
>>>> -     * Sum of res_postcopy_only, res_compatible and
>>>> res_postcopy_only is the
>>>> -     * whole amount of pending data.
>>>> +     * Sum of res_precopy and res_postcopy is the whole amount of
>>>> +     * pending data.
>>>>         */
>>>>
>>>>
>>> [..]
>>>
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index dc1de9ddbc..20167e1102 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -3435,9 +3435,7 @@ static int ram_save_complete(QEMUFile *f, void
>>>> *opaque)
>>>>    }
>>>>
>>>>    static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t
>>>> max_size,
>>>> -                             uint64_t *res_precopy_only,
>>>> -                             uint64_t *res_compatible,
>>>> -                             uint64_t *res_postcopy_only)
>>>> +                             uint64_t *res_precopy, uint64_t
>>>> *res_postcopy)
>>>>    {
>>>>        RAMState **temp = opaque;
>>>>        RAMState *rs = *temp;
>>>> @@ -3457,9 +3455,9 @@ static void ram_save_pending(QEMUFile *f, void
>>>> *opaque, uint64_t max_size,
>>>>
>>>>        if (migrate_postcopy_ram()) {
>>>>            /* We can do postcopy, and all the data is postcopiable */
>>>> -        *res_compatible += remaining_size;
>>>> +        *res_postcopy += remaining_size;
>>> That's seems to be not quite correct.
>>>
>>> res_postcopy is defined as "data which must be migrated in postcopy",
>>> but that's not true here, as RAM can be migrated both in precopy and
>>> postcopy.
>>>
>>> Still we really can include "compat" into "postcopy" just because in the
>>> logic of migration_iteration_run() we don't actually distinguish
>>> "compat" and "post". The logic only depends on "total" and "pre".
>>>
>>> So, if we want to combine "compat" into "post", we should redefine
>>> "post" in the comment in include/migration/register.h, something like
>>> this:
>>>
>>> - res_precopy is for data which MUST be migrated in precopy
>>>    phase or in stopped state, in other words - before target
>>>    vm start
>>>
>>> - res_postcopy is for all data except for declared in res_precopy.
>>>    res_postcopy data CAN be migrated in postcopy, i.e. after target
>>>    vm start.
>>>
>>>
>> You are right, the definition of res_postcopy should be changed.
>>
>> Yet, I am not sure if this patch really makes things more clear/simple.
>> Juan, what do you think?
>>
>> Thanks!
>>>>        } else {
>>>> -        *res_precopy_only += remaining_size;
>>>> +        *res_precopy += remaining_size;
>>>>        }
>>>>    }
>>>>
>>>
>>> --
>>> Best regards,
>>> Vladimir
>>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>