Re: [PATCH 05/11] migration: Drop save_live_complete_postcopy hook
On Thu, May 29, 2025 at 04:48:41PM +0200, Juraj Marcin wrote:
> On 2025-05-29 16:42, Juraj Marcin wrote:
> > Hi Peter
Hey, Juraj!
> >
> > On 2025-05-27 17:58, Peter Xu wrote:
> > > The hook is only defined in two vmstate users ("ram" and "block dirty
> > > bitmap"), meanwhile both of them define the hook exactly the same as the
> > > precopy version. Hence, this postcopy version isn't needed.
> > >
> > > No functional change intended.
> >
> > Could be some future users, that would benefit from separate hooks for
> > precopy and postcopy?
There's nothing that I'm aware of. Normally we should only provide
separate hook until that comes, and then review the hook together with the
use case.
> >
> > In case we are going to drop it, I think the '_precopy' suffix could be
> > dropped too, as the handler would be used for postcopy too.
>
> Never mind, just noticed it's in the next patch...
Yep. I can squash that into this one; it's pretty small so maybe it's
fine. I'll keep it separate as of now, but if anyone thinks it better I
can squash them.
--
Peter Xu
Re: [PATCH 05/11] migration: Drop save_live_complete_postcopy hook
On 2025-05-29 16:42, Juraj Marcin wrote:
> Hi Peter
>
> On 2025-05-27 17:58, Peter Xu wrote:
> > The hook is only defined in two vmstate users ("ram" and "block dirty
> > bitmap"), meanwhile both of them define the hook exactly the same as the
> > precopy version. Hence, this postcopy version isn't needed.
> >
> > No functional change intended.
>
> Could be some future users, that would benefit from separate hooks for
> precopy and postcopy?
>
> In case we are going to drop it, I think the '_precopy' suffix could be
> dropped too, as the handler would be used for postcopy too.
Never mind, just noticed it's in the next patch...
>
> Best regards
>
> Juraj Marcin
>
> >
> > Signed-off-by: Peter Xu
> > ---
> > include/migration/register.h | 24
> > migration/block-dirty-bitmap.c | 1 -
> > migration/ram.c| 1 -
> > migration/savevm.c | 9 -
> > 4 files changed, 12 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/migration/register.h b/include/migration/register.h
> > index b79dc81b8d..e022195785 100644
> > --- a/include/migration/register.h
> > +++ b/include/migration/register.h
> > @@ -77,26 +77,18 @@ typedef struct SaveVMHandlers {
> > */
> > void (*save_cleanup)(void *opaque);
> >
> > -/**
> > - * @save_live_complete_postcopy
> > - *
> > - * Called at the end of postcopy for all postcopyable devices.
> > - *
> > - * @f: QEMUFile where to send the data
> > - * @opaque: data pointer passed to register_savevm_live()
> > - *
> > - * Returns zero to indicate success and negative for error
> > - */
> > -int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
> > -
> > /**
> > * @save_live_complete_precopy
> > *
> > * Transmits the last section for the device containing any
> > - * remaining data at the end of a precopy phase. When postcopy is
> > - * enabled, devices that support postcopy will skip this step,
> > - * where the final data will be flushed at the end of postcopy via
> > - * @save_live_complete_postcopy instead.
> > + * remaining data at the end phase of migration.
> > + *
> > + * For precopy, this will be invoked _during_ the switchover phase
> > + * after source VM is stopped.
> > + *
> > + * For postcopy, this will be invoked _after_ the switchover phase
> > + * (except some very unusual cases, like PMEM ramblocks), while
> > + * destination VM can be running.
> > *
> > * @f: QEMUFile where to send the data
> > * @opaque: data pointer passed to register_savevm_live()
> > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> > index f2c352d4a7..6ee3c32a76 100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -1248,7 +1248,6 @@ static bool dirty_bitmap_has_postcopy(void *opaque)
> >
> > static SaveVMHandlers savevm_dirty_bitmap_handlers = {
> > .save_setup = dirty_bitmap_save_setup,
> > -.save_live_complete_postcopy = dirty_bitmap_save_complete,
> > .save_live_complete_precopy = dirty_bitmap_save_complete,
> > .has_postcopy = dirty_bitmap_has_postcopy,
> > .state_pending_exact = dirty_bitmap_state_pending,
> > diff --git a/migration/ram.c b/migration/ram.c
> > index fd8d83b63c..8b43b9e1e8 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -4545,7 +4545,6 @@ void postcopy_preempt_shutdown_file(MigrationState *s)
> > static SaveVMHandlers savevm_ram_handlers = {
> > .save_setup = ram_save_setup,
> > .save_live_iterate = ram_save_iterate,
> > -.save_live_complete_postcopy = ram_save_complete,
> > .save_live_complete_precopy = ram_save_complete,
> > .has_postcopy = ram_has_postcopy,
> > .state_pending_exact = ram_state_pending_exact,
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 006514c3e3..26d32eb5a7 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1485,9 +1485,8 @@ bool should_send_vmdesc(void)
> > }
> >
> > /*
> > - * Calls the save_live_complete_postcopy methods
> > - * causing the last few pages to be sent immediately and doing any
> > associated
> > - * cleanup.
> > + * Complete saving any postcopy-able devices.
> > + *
> > * Note postcopy also calls qemu_savevm_state_complete_precopy to complete
> > * all the other devices, but that happens at the point we switch to
> > postcopy.
> > */
> > @@ -1497,7 +1496,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
> > int ret;
> >
> > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> > -if (!se->ops || !se->ops->save_live_complete_postcopy) {
> > +if (!se->ops || !se->ops->save_live_complete_precopy) {
> > continue;
> > }
> > if (se->ops->is_active) {
> > @@ -1510,7 +1509,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
> > qemu_put_b
Re: [PATCH 05/11] migration: Drop save_live_complete_postcopy hook
Hi Peter
On 2025-05-27 17:58, Peter Xu wrote:
> The hook is only defined in two vmstate users ("ram" and "block dirty
> bitmap"), meanwhile both of them define the hook exactly the same as the
> precopy version. Hence, this postcopy version isn't needed.
>
> No functional change intended.
Could be some future users, that would benefit from separate hooks for
precopy and postcopy?
In case we are going to drop it, I think the '_precopy' suffix could be
dropped too, as the handler would be used for postcopy too.
Best regards
Juraj Marcin
>
> Signed-off-by: Peter Xu
> ---
> include/migration/register.h | 24
> migration/block-dirty-bitmap.c | 1 -
> migration/ram.c| 1 -
> migration/savevm.c | 9 -
> 4 files changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/include/migration/register.h b/include/migration/register.h
> index b79dc81b8d..e022195785 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -77,26 +77,18 @@ typedef struct SaveVMHandlers {
> */
> void (*save_cleanup)(void *opaque);
>
> -/**
> - * @save_live_complete_postcopy
> - *
> - * Called at the end of postcopy for all postcopyable devices.
> - *
> - * @f: QEMUFile where to send the data
> - * @opaque: data pointer passed to register_savevm_live()
> - *
> - * Returns zero to indicate success and negative for error
> - */
> -int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque);
> -
> /**
> * @save_live_complete_precopy
> *
> * Transmits the last section for the device containing any
> - * remaining data at the end of a precopy phase. When postcopy is
> - * enabled, devices that support postcopy will skip this step,
> - * where the final data will be flushed at the end of postcopy via
> - * @save_live_complete_postcopy instead.
> + * remaining data at the end phase of migration.
> + *
> + * For precopy, this will be invoked _during_ the switchover phase
> + * after source VM is stopped.
> + *
> + * For postcopy, this will be invoked _after_ the switchover phase
> + * (except some very unusual cases, like PMEM ramblocks), while
> + * destination VM can be running.
> *
> * @f: QEMUFile where to send the data
> * @opaque: data pointer passed to register_savevm_live()
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index f2c352d4a7..6ee3c32a76 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -1248,7 +1248,6 @@ static bool dirty_bitmap_has_postcopy(void *opaque)
>
> static SaveVMHandlers savevm_dirty_bitmap_handlers = {
> .save_setup = dirty_bitmap_save_setup,
> -.save_live_complete_postcopy = dirty_bitmap_save_complete,
> .save_live_complete_precopy = dirty_bitmap_save_complete,
> .has_postcopy = dirty_bitmap_has_postcopy,
> .state_pending_exact = dirty_bitmap_state_pending,
> diff --git a/migration/ram.c b/migration/ram.c
> index fd8d83b63c..8b43b9e1e8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4545,7 +4545,6 @@ void postcopy_preempt_shutdown_file(MigrationState *s)
> static SaveVMHandlers savevm_ram_handlers = {
> .save_setup = ram_save_setup,
> .save_live_iterate = ram_save_iterate,
> -.save_live_complete_postcopy = ram_save_complete,
> .save_live_complete_precopy = ram_save_complete,
> .has_postcopy = ram_has_postcopy,
> .state_pending_exact = ram_state_pending_exact,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 006514c3e3..26d32eb5a7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1485,9 +1485,8 @@ bool should_send_vmdesc(void)
> }
>
> /*
> - * Calls the save_live_complete_postcopy methods
> - * causing the last few pages to be sent immediately and doing any associated
> - * cleanup.
> + * Complete saving any postcopy-able devices.
> + *
> * Note postcopy also calls qemu_savevm_state_complete_precopy to complete
> * all the other devices, but that happens at the point we switch to
> postcopy.
> */
> @@ -1497,7 +1496,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
> int ret;
>
> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> -if (!se->ops || !se->ops->save_live_complete_postcopy) {
> +if (!se->ops || !se->ops->save_live_complete_precopy) {
> continue;
> }
> if (se->ops->is_active) {
> @@ -1510,7 +1509,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
> qemu_put_byte(f, QEMU_VM_SECTION_END);
> qemu_put_be32(f, se->section_id);
>
> -ret = se->ops->save_live_complete_postcopy(f, se->opaque);
> +ret = se->ops->save_live_complete_precopy(f, se->opaque);
> trace_savevm_section_end(se->idstr, se->section_id, ret);
> save_section_footer(f, se);
> i
