Re: [PATCH 4/4] migration: Introduce POSTCOPY_DEVICE state

[Peter Xu <peterx@redhat.com>]

On Fri, Sep 19, 2025 at 12:58:08PM -0400, Peter Xu wrote:
> > @@ -2564,6 +2569,11 @@ static void *source_return_path_thread(void *opaque)
> >              tmp32 = ldl_be_p(buf);
> >              trace_source_return_path_thread_pong(tmp32);
> >              qemu_sem_post(&ms->rp_state.rp_pong_acks);
> > +            if (tmp32 == QEMU_VM_PING_PACKAGED_LOADED) {
> > +                trace_source_return_path_thread_dst_started();
> > +                migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_DEVICE,
> > +                                  MIGRATION_STATUS_POSTCOPY_ACTIVE);
> 
> Could this race with the migration thread modifying the state concurrently?
> 
> To avoid it, we could have a bool, set it here once, and in the iterations
> do something like:
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25d..55230e10ee 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3449,6 +3449,16 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>      trace_migrate_pending_estimate(pending_size, must_precopy, can_postcopy);
>  
>      if (in_postcopy) {
> +        if (s->postcopy_package_loaded) {
> +            assert(s->state == MIGRATION_STATUS_POSTCOPY_DEVICE);
> +            migrate_set_state(s->state, MIGRATION_STATUS_POSTCOPY_DEVICE,
> +                              MIGRATION_STATUS_POSTCOPY_ACTIVE);
> +            /*
> +             * Since postcopy cannot be re-initiated, this flag will only
> +             * be set at most once for QEMU's whole lifecyce.
> +             */
> +            s->postcopy_package_loaded = false;
> +        }
>          /*
>           * Iterate in postcopy until all pending data flushed.  Note that
>           * postcopy completion doesn't rely on can_switchover, because when

[...]

> > @@ -2871,7 +2882,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
> >  
> >      /* Now, switchover looks all fine, switching to postcopy-active */
> >      migrate_set_state(&ms->state, MIGRATION_STATUS_DEVICE,
> > -                      MIGRATION_STATUS_POSTCOPY_ACTIVE);
> > +                      MIGRATION_STATUS_POSTCOPY_DEVICE);
> >  
> >      bql_unlock();
> >  
> > @@ -3035,7 +3046,8 @@ static void migration_completion(MigrationState *s)
> >  
> >      if (s->state == MIGRATION_STATUS_ACTIVE) {
> >          ret = migration_completion_precopy(s);
> > -    } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> > +    } else if (s->state == MIGRATION_STATUS_POSTCOPY_DEVICE ||
> > +               s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> 
> Exactly.  We need to be prepared that src sending too fast so when device
> loading on dest we finished.

One thing more to mention here.. which may void some previous comments I
left.  Let's discuss.

I think it may also make sense to only allow a COMPLETE after
POSTCOPY_ACTIVE.

That is, if src sends too fast to have finished sending everything,
reaching COMPLETE during POSTCOPY_DEVICE, that is, while before it receives
the new PONG you defined, then.. I _think_ it is better to wait for that.

If it finally arrives, then it's perfect, we switch to POSTCOPY_ACTIVE,
then continue the completion.

If the channel is broken before its arrival, logically we should handle
this case as a FAILURE and restart the VM on src.

It's only relevant in a corner case, but does that sound better?

-- 
Peter Xu