| Message ID | 20230208135719.17864-4-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove res_compatible parameter | expand |
On 08.02.23 16:57, Juan Quintela wrote: > Once that res_compatible is removed, they don't make sense anymore. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > include/migration/register.h | 18 ++++++++---------- > migration/savevm.h | 8 ++++---- > hw/s390x/s390-stattrib.c | 7 +++---- > hw/vfio/migration.c | 10 ++++------ > migration/block-dirty-bitmap.c | 6 +++--- > migration/block.c | 7 +++---- > migration/ram.c | 18 ++++++++---------- > migration/savevm.c | 24 ++++++++++-------------- > 8 files changed, 43 insertions(+), 55 deletions(-) > > diff --git a/include/migration/register.h b/include/migration/register.h > index a958a92a0f..4a4a6d7174 100644 > --- a/include/migration/register.h > +++ b/include/migration/register.h > @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers { > /* This runs outside the iothread lock! */ > int (*save_setup)(QEMUFile *f, void *opaque); > /* Note for save_live_pending: > - * - res_precopy_only is for data which must be migrated in precopy phase > + * - 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_only is for data which must be migrated in postcopy phase > + * - res_postcopy is for data which must be migrated in postcopy phase > * or in stopped state, in other words - after source vm stop That's now wrong. "postcopy" is everything except "precopy", as it includes "compat". Really, for RAM, it can be copied in precopy too, and it is copied in precopy until user run command migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot migrate in precopy at all, it migrate only in stopped state or in postcopy). So, finally: "precopy" definition: - must be migrated in precopy or in stopped state - in other words: must be migrated before target start - in other words: can't be migrated in postcopy - in other words: can't be migrated after target start "postcopy" definition: - can migrate in postcopy - in other words: can migrate after target start some properties: - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap) - of course, can be migrated in stopped state To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable".
-----Original Message----- From: qemu-s390x-bounces+jadon.mcdowell=optum.com@nongnu.org <qemu-s390x-bounces+jadon.mcdowell=optum.com@nongnu.org> On Behalf Of Vladimir Sementsov-Ogievskiy Sent: Tuesday, February 14, 2023 9:27 AM To: Juan Quintela <quintela@redhat.com>; qemu-devel@nongnu.org Cc: Ilya Leoshkevich <iii@linux.ibm.com>; qemu-block@nongnu.org; Alex Williamson <alex.williamson@redhat.com>; Fam Zheng <fam@euphon.net>; Eric Blake <eblake@redhat.com>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Halil Pasic <pasic@linux.ibm.com>; David Hildenbrand <david@redhat.com>; Thomas Huth <thuth@redhat.com>; Eric Farman <farman@linux.ibm.com>; qemu-s390x@nongnu.org; John Snow <jsnow@redhat.com>; Christian Borntraeger <borntraeger@linux.ibm.com>; Richard Henderson <richard.henderson@linaro.org> Subject: Re: [PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy On 08.02.23 16:57, Juan Quintela wrote: > Once that res_compatible is removed, they don't make sense anymore. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > include/migration/register.h | 18 ++++++++---------- > migration/savevm.h | 8 ++++---- > hw/s390x/s390-stattrib.c | 7 +++---- > hw/vfio/migration.c | 10 ++++------ > migration/block-dirty-bitmap.c | 6 +++--- > migration/block.c | 7 +++---- > migration/ram.c | 18 ++++++++---------- > migration/savevm.c | 24 ++++++++++-------------- > 8 files changed, 43 insertions(+), 55 deletions(-) > > diff --git a/include/migration/register.h > b/include/migration/register.h index a958a92a0f..4a4a6d7174 100644 > --- a/include/migration/register.h > +++ b/include/migration/register.h > @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers { > /* This runs outside the iothread lock! */ > int (*save_setup)(QEMUFile *f, void *opaque); > /* Note for save_live_pending: > - * - res_precopy_only is for data which must be migrated in precopy phase > + * - 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_only is for data which must be migrated in postcopy phase > + * - res_postcopy is for data which must be migrated in postcopy > + phase > * or in stopped state, in other words - after source vm stop That's now wrong. "postcopy" is everything except "precopy", as it includes "compat". Really, for RAM, it can be copied in precopy too, and it is copied in precopy until user run command migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot migrate in precopy at all, it migrate only in stopped state or in postcopy). So, finally: "precopy" definition: - must be migrated in precopy or in stopped state - in other words: must be migrated before target start - in other words: can't be migrated in postcopy - in other words: can't be migrated after target start "postcopy" definition: - can migrate in postcopy - in other words: can migrate after target start some properties: - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap) - of course, can be migrated in stopped state To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable". -- Best regards, Vladimir This e-mail, including attachments, may include confidential and/or proprietary information, and may be used only by the person or entity to which it is addressed. If the reader of this e-mail is not the intended recipient or intended recipient’s authorized agent, the reader is hereby notified that any dissemination, distribution or copying of this e-mail is prohibited. If you have received this e-mail in error, please notify the sender by replying to this message and delete this e-mail immediately.
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > On 08.02.23 16:57, Juan Quintela wrote: >> Once that res_compatible is removed, they don't make sense anymore. >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> include/migration/register.h | 18 ++++++++---------- >> migration/savevm.h | 8 ++++---- >> hw/s390x/s390-stattrib.c | 7 +++---- >> hw/vfio/migration.c | 10 ++++------ >> migration/block-dirty-bitmap.c | 6 +++--- >> migration/block.c | 7 +++---- >> migration/ram.c | 18 ++++++++---------- >> migration/savevm.c | 24 ++++++++++-------------- >> 8 files changed, 43 insertions(+), 55 deletions(-) >> diff --git a/include/migration/register.h >> b/include/migration/register.h >> index a958a92a0f..4a4a6d7174 100644 >> --- a/include/migration/register.h >> +++ b/include/migration/register.h >> @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers { >> /* This runs outside the iothread lock! */ >> int (*save_setup)(QEMUFile *f, void *opaque); >> /* Note for save_live_pending: >> - * - res_precopy_only is for data which must be migrated in precopy phase >> + * - 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_only is for data which must be migrated in postcopy phase >> + * - res_postcopy is for data which must be migrated in postcopy phase >> * or in stopped state, in other words - after source vm stop > > > That's now wrong. "postcopy" is everything except "precopy", as it > includes "compat". Really, for RAM, it can be copied in precopy too, > and it is copied in precopy until user run command > migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot > migrate in precopy at all, it migrate only in stopped state or in > postcopy). > > So, finally: > > "precopy" > > definition: > - must be migrated in precopy or in stopped state > - in other words: must be migrated before target start > - in other words: can't be migrated in postcopy > - in other words: can't be migrated after target start > > "postcopy" > > definition: > - can migrate in postcopy > - in other words: can migrate after target start > some properties: > - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap) > - of course, can be migrated in stopped state What about (with latest naming) must_precopy: * must be migrated in precopy or in stopped state * i.e. must be migrated before target start can_postcopy: * can migrate in postcopy or in stopped state * i.e. can migrate after target start * some can also be migrated during precopy (RAM) * some must be migrated after source stops (block-dirty-bitmap) > To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable". I hate negatives when naming variables. What about: must_precopy and can_postcopy? Later, Juan.
On 14.02.23 21:22, Juan Quintela wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: >> On 08.02.23 16:57, Juan Quintela wrote: >>> Once that res_compatible is removed, they don't make sense anymore. >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> include/migration/register.h | 18 ++++++++---------- >>> migration/savevm.h | 8 ++++---- >>> hw/s390x/s390-stattrib.c | 7 +++---- >>> hw/vfio/migration.c | 10 ++++------ >>> migration/block-dirty-bitmap.c | 6 +++--- >>> migration/block.c | 7 +++---- >>> migration/ram.c | 18 ++++++++---------- >>> migration/savevm.c | 24 ++++++++++-------------- >>> 8 files changed, 43 insertions(+), 55 deletions(-) >>> diff --git a/include/migration/register.h >>> b/include/migration/register.h >>> index a958a92a0f..4a4a6d7174 100644 >>> --- a/include/migration/register.h >>> +++ b/include/migration/register.h >>> @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers { >>> /* This runs outside the iothread lock! */ >>> int (*save_setup)(QEMUFile *f, void *opaque); >>> /* Note for save_live_pending: >>> - * - res_precopy_only is for data which must be migrated in precopy phase >>> + * - 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_only is for data which must be migrated in postcopy phase >>> + * - res_postcopy is for data which must be migrated in postcopy phase >>> * or in stopped state, in other words - after source vm stop >> >> >> That's now wrong. "postcopy" is everything except "precopy", as it >> includes "compat". Really, for RAM, it can be copied in precopy too, >> and it is copied in precopy until user run command >> migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot >> migrate in precopy at all, it migrate only in stopped state or in >> postcopy). >> >> So, finally: >> >> "precopy" >> >> definition: >> - must be migrated in precopy or in stopped state >> - in other words: must be migrated before target start >> - in other words: can't be migrated in postcopy >> - in other words: can't be migrated after target start >> >> "postcopy" >> >> definition: >> - can migrate in postcopy >> - in other words: can migrate after target start >> some properties: >> - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap) >> - of course, can be migrated in stopped state > > What about (with latest naming) > > must_precopy: > * must be migrated in precopy or in stopped state > * i.e. must be migrated before target start > > can_postcopy: > * can migrate in postcopy or in stopped state > * i.e. can migrate after target start > * some can also be migrated during precopy (RAM) > * some must be migrated after source stops (block-dirty-bitmap) Sounds very good, I'm for! > >> To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable". > > I hate negatives when naming variables. Agree. > > What about: must_precopy and can_postcopy? > Sounds good!)
diff --git a/include/migration/register.h b/include/migration/register.h index a958a92a0f..4a4a6d7174 100644 --- a/include/migration/register.h +++ b/include/migration/register.h @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers { /* This runs outside the iothread lock! */ int (*save_setup)(QEMUFile *f, void *opaque); /* Note for save_live_pending: - * - res_precopy_only is for data which must be migrated in precopy phase + * - 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_only is for data which must be migrated in postcopy phase + * - 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 and res_postcopy_only is the whole - * amount of pending data. + * Sum of res_postcopy and res_postcopy is the whole amount of + * pending data. */ /* This estimates the remaining data to transfer */ - void (*state_pending_estimate)(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only); + void (*state_pending_estimate)(void *opaque, uint64_t *res_precopy, + uint64_t *res_postcopy); /* This calculate the exact remaining data to transfer */ - void (*state_pending_exact)(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only); + void (*state_pending_exact)(void *opaque, uint64_t *res_precopy, + uint64_t *res_postcopy); LoadStateHandler *load_state; int (*load_setup)(QEMUFile *f, void *opaque); int (*load_cleanup)(void *opaque); diff --git a/migration/savevm.h b/migration/savevm.h index bd625a644b..24f2d2a28b 100644 --- a/migration/savevm.h +++ b/migration/savevm.h @@ -40,10 +40,10 @@ void qemu_savevm_state_cleanup(void); 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_exact(uint64_t *res_precopy_only, - uint64_t *res_postcopy_only); -void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only, - uint64_t *res_postcopy_only); +void qemu_savevm_state_pending_exact(uint64_t *res_precopy, + uint64_t *res_postcopy); +void qemu_savevm_state_pending_estimate(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/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c index c7ae9184ab..dfb95eb20c 100644 --- a/hw/s390x/s390-stattrib.c +++ b/hw/s390x/s390-stattrib.c @@ -182,16 +182,15 @@ static int cmma_save_setup(QEMUFile *f, void *opaque) return 0; } -static void cmma_state_pending(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +static void cmma_state_pending(void *opaque, 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 fb0dd9571d..d8c913acde 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -456,9 +456,8 @@ static void vfio_save_cleanup(void *opaque) trace_vfio_save_cleanup(vbasedev->name); } -static void vfio_state_pending(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +static void vfio_state_pending(void *opaque, uint64_t *res_precopy, + uint64_t *res_postcopy) { VFIODevice *vbasedev = opaque; VFIOMigration *migration = vbasedev->migration; @@ -469,10 +468,9 @@ static void vfio_state_pending(void *opaque, return; } - *res_precopy_only += migration->pending_bytes; + *res_precopy += migration->pending_bytes; - trace_vfio_state_pending(vbasedev->name, *res_precopy_only, - *res_postcopy_only); + trace_vfio_state_pending(vbasedev->name, *res_precopy, *res_postcopy); } static int vfio_save_iterate(QEMUFile *f, void *opaque) diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 9c6655e11a..793bf4347d 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -763,8 +763,8 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque) } static void dirty_bitmap_state_pending(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) + uint64_t *res_precopy, + uint64_t *res_postcopy) { DBMSaveState *s = &((DBMState *)opaque)->save; SaveBitmapState *dbms; @@ -784,7 +784,7 @@ static void dirty_bitmap_state_pending(void *opaque, trace_dirty_bitmap_state_pending(pending); - *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 99d149904c..21bcf560e0 100644 --- a/migration/block.c +++ b/migration/block.c @@ -863,9 +863,8 @@ static int block_save_complete(QEMUFile *f, void *opaque) return 0; } -static void block_state_pending(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +static void block_state_pending(void *opaque, 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_state_pending(void *opaque, trace_migration_block_state_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/ram.c b/migration/ram.c index abd960659c..add5c5edfc 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3435,9 +3435,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque) return 0; } -static void ram_state_pending_estimate(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +static void ram_state_pending_estimate(void *opaque, uint64_t *res_precopy, + uint64_t *res_postcopy) { RAMState **temp = opaque; RAMState *rs = *temp; @@ -3446,15 +3445,14 @@ static void ram_state_pending_estimate(void *opaque, if (migrate_postcopy_ram()) { /* We can do postcopy, and all the data is postcopiable */ - *res_postcopy_only += remaining_size; + *res_postcopy += remaining_size; } else { - *res_precopy_only += remaining_size; + *res_precopy += remaining_size; } } -static void ram_state_pending_exact(void *opaque, - uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +static void ram_state_pending_exact(void *opaque, uint64_t *res_precopy, + uint64_t *res_postcopy) { RAMState **temp = opaque; RAMState *rs = *temp; @@ -3472,9 +3470,9 @@ static void ram_state_pending_exact(void *opaque, if (migrate_postcopy_ram()) { /* We can do postcopy, and all the data is postcopiable */ - *res_postcopy_only += 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 2943b8301b..9d3d971a50 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1541,13 +1541,13 @@ flush: * the result is split into the amount for units that can and * for units that can't do postcopy. */ -void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +void qemu_savevm_state_pending_estimate(uint64_t *res_precopy, + uint64_t *res_postcopy) { SaveStateEntry *se; - *res_precopy_only = 0; - *res_postcopy_only = 0; + *res_precopy = 0; + *res_postcopy = 0; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (!se->ops || !se->ops->state_pending_exact) { @@ -1558,19 +1558,17 @@ void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only, continue; } } - se->ops->state_pending_exact(se->opaque, - res_precopy_only, - res_postcopy_only); + se->ops->state_pending_exact(se->opaque, res_precopy, res_postcopy); } } -void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only, - uint64_t *res_postcopy_only) +void qemu_savevm_state_pending_exact(uint64_t *res_precopy, + uint64_t *res_postcopy) { SaveStateEntry *se; - *res_precopy_only = 0; - *res_postcopy_only = 0; + *res_precopy = 0; + *res_postcopy = 0; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (!se->ops || !se->ops->state_pending_estimate) { @@ -1581,9 +1579,7 @@ void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only, continue; } } - se->ops->state_pending_estimate(se->opaque, - res_precopy_only, - res_postcopy_only); + se->ops->state_pending_estimate(se->opaque, res_precopy, res_postcopy); } }
Once that res_compatible is removed, they don't make sense anymore. Signed-off-by: Juan Quintela <quintela@redhat.com> --- include/migration/register.h | 18 ++++++++---------- migration/savevm.h | 8 ++++---- hw/s390x/s390-stattrib.c | 7 +++---- hw/vfio/migration.c | 10 ++++------ migration/block-dirty-bitmap.c | 6 +++--- migration/block.c | 7 +++---- migration/ram.c | 18 ++++++++---------- migration/savevm.c | 24 ++++++++++-------------- 8 files changed, 43 insertions(+), 55 deletions(-)