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

[Avihai Horon <avihaih@nvidia.com>]

Ping.

On 10/11/2022 15:36, Avihai Horon 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.".
>>> 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
>>