| Message ID | 20221103161620.13120-2-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/migration: Implement VFIO migration protocol v2 | expand |
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; > } > } >
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 >
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 >>
* 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. 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 > > >
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 >
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c index 9eda1c3b2a..ee60b53da4 100644 --- a/hw/s390x/s390-stattrib.c +++ b/hw/s390x/s390-stattrib.c @@ -183,16 +183,14 @@ static int cmma_save_setup(QEMUFile *f, void *opaque) } static void cmma_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) { S390StAttribState *sas = S390_STATTRIB(opaque); S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas); long long res = sac->get_dirtycount(sas); if (res >= 0) { - *res_precopy_only += res; + *res_precopy += res; } } diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 3de4252111..3423f113f0 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -458,9 +458,8 @@ static void vfio_save_cleanup(void *opaque) static void vfio_save_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 *res_precopy, + uint64_t *res_postcopy) { VFIODevice *vbasedev = opaque; VFIOMigration *migration = vbasedev->migration; @@ -471,10 +470,9 @@ static void vfio_save_pending(QEMUFile *f, void *opaque, return; } - *res_precopy_only += migration->pending_bytes; + *res_precopy += migration->pending_bytes; - trace_vfio_save_pending(vbasedev->name, *res_precopy_only, - *res_postcopy_only, *res_compatible); + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); } static int vfio_save_iterate(QEMUFile *f, void *opaque) diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index 73dffe9e00..a21cbd2a56 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -157,7 +157,7 @@ vfio_save_cleanup(const char *name) " (%s)" vfio_save_buffer(const char *name, uint64_t data_offset, uint64_t data_size, uint64_t pending) " (%s) Offset 0x%"PRIx64" size 0x%"PRIx64" pending 0x%"PRIx64 vfio_update_pending(const char *name, uint64_t pending) " (%s) pending 0x%"PRIx64 vfio_save_device_config_state(const char *name) " (%s)" -vfio_save_pending(const char *name, uint64_t precopy, uint64_t postcopy, uint64_t compatible) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64" compatible 0x%"PRIx64 +vfio_save_pending(const char *name, uint64_t precopy, uint64_t postcopy) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64 vfio_save_iterate(const char *name, int data_size) " (%s) data_size %d" vfio_save_complete_precopy(const char *name) " (%s)" vfio_load_device_config_state(const char *name) " (%s)" 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/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 9aba7d9c22..dfea546330 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -763,9 +763,8 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque) static void dirty_bitmap_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) { DBMSaveState *s = &((DBMState *)opaque)->save; SaveBitmapState *dbms; @@ -785,7 +784,7 @@ static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque, trace_dirty_bitmap_save_pending(pending, max_size); - *res_postcopy_only += pending; + *res_postcopy += pending; } /* First occurrence of this bitmap. It should be created if doesn't exist */ diff --git a/migration/block.c b/migration/block.c index 3577c815a9..4ae8f837b0 100644 --- a/migration/block.c +++ b/migration/block.c @@ -863,9 +863,8 @@ static int block_save_complete(QEMUFile *f, void *opaque) } static void block_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) { /* Estimate pending number of bytes to send */ uint64_t pending; @@ -886,7 +885,7 @@ static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size, trace_migration_block_save_pending(pending); /* We don't do postcopy */ - *res_precopy_only += pending; + *res_precopy += pending; } static int block_load(QEMUFile *f, void *opaque, int version_id) diff --git a/migration/migration.c b/migration/migration.c index 739bb683f3..a4a18228c6 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3735,15 +3735,14 @@ typedef enum { */ static MigIterateState migration_iteration_run(MigrationState *s) { - uint64_t pending_size, pend_pre, pend_compat, pend_post; + uint64_t pending_size, pend_pre, pend_post; bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; qemu_savevm_state_pending(s->to_dst_file, s->threshold_size, &pend_pre, - &pend_compat, &pend_post); - pending_size = pend_pre + pend_compat + pend_post; + &pend_post); + pending_size = pend_pre + pend_post; - trace_migrate_pending(pending_size, s->threshold_size, - pend_pre, pend_compat, pend_post); + trace_migrate_pending(pending_size, s->threshold_size, pend_pre, pend_post); if (pending_size && pending_size >= s->threshold_size) { /* Still a significant amount to transfer */ 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; } else { - *res_precopy_only += remaining_size; + *res_precopy += remaining_size; } } diff --git a/migration/savevm.c b/migration/savevm.c index a0cdb714f7..4d02887f25 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1472,16 +1472,13 @@ flush: * for units that can't do postcopy. */ void qemu_savevm_state_pending(QEMUFile *f, uint64_t threshold_size, - uint64_t *res_precopy_only, - uint64_t *res_compatible, - uint64_t *res_postcopy_only) + uint64_t *res_precopy, + uint64_t *res_postcopy) { SaveStateEntry *se; - *res_precopy_only = 0; - *res_compatible = 0; - *res_postcopy_only = 0; - + *res_precopy = 0; + *res_postcopy = 0; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (!se->ops || !se->ops->save_live_pending) { @@ -1493,8 +1490,7 @@ void qemu_savevm_state_pending(QEMUFile *f, uint64_t threshold_size, } } se->ops->save_live_pending(f, se->opaque, threshold_size, - res_precopy_only, res_compatible, - res_postcopy_only); + res_precopy, res_postcopy); } } diff --git a/migration/savevm.h b/migration/savevm.h index 6461342cb4..9bd55c336c 100644 --- a/migration/savevm.h +++ b/migration/savevm.h @@ -41,9 +41,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f); int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only, bool inactivate_disks); void qemu_savevm_state_pending(QEMUFile *f, 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); void qemu_savevm_send_ping(QEMUFile *f, uint32_t value); void qemu_savevm_send_open_return_path(QEMUFile *f); int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len); diff --git a/migration/trace-events b/migration/trace-events index 57003edcbd..f2a873fd6c 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -150,7 +150,7 @@ migrate_fd_cleanup(void) "" migrate_fd_error(const char *error_desc) "error=%s" migrate_fd_cancel(void) "" migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx" -migrate_pending(uint64_t size, uint64_t max, uint64_t pre, uint64_t compat, uint64_t post) "pending size %" PRIu64 " max %" PRIu64 " (pre = %" PRIu64 " compat=%" PRIu64 " post=%" PRIu64 ")" +migrate_pending(uint64_t size, uint64_t max, uint64_t pre, uint64_t post) "pending size %" PRIu64 " max %" PRIu64 " (pre = %" PRIu64 " post=%" PRIu64 ")" migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d" migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64 migration_completion_file_err(void) ""