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

[Avihai Horon]

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