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

[Vladimir Sementsov-Ogievskiy]

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.

> 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.


>       } else {
> -        *res_precopy_only += remaining_size;
> +        *res_precopy += remaining_size;
>       }
>   }
>   


--
Best regards,
Vladimir