* Avihai Horon (avih...@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 <quint...@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. Dave > > > Just delete the field in all callers. > > > > > > Signed-off-by: Juan Quintela <quint...@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 / dgilb...@redhat.com / Manchester, UK