Re: [PATCH v1] migration: refactor migration_completion

[Peter Xu <peterx@redhat.com>]

On Tue, Jul 18, 2023 at 01:25:12PM +0000, Wang, Wei W wrote:
> On Tuesday, July 18, 2023 1:44 PM, Isaku Yamahata wrote:
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -2058,6 +2058,21 @@ static int
> > await_return_path_close_on_source(MigrationState *ms)
> > >      return ms->rp_state.error;
> > >  }
> > >
> > > +static int close_return_path_on_source(MigrationState *ms) {
> > > +    int ret;
> > > +
> > > +    if (!ms->rp_state.rp_thread_created) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    trace_migration_return_path_end_before();
> > > +    ret = await_return_path_close_on_source(ms);
> > > +    trace_migration_return_path_end_after(ret);
> > > +
> > > +    return ret;
> > > +}
> > > +
> > 
> > There is only one caller, migration_completion().  We can consolidate two
> > functions, await_return_path_close_on_source() and
> > close_return_path_on_source(), into single function.
> 
> Sounds good, thanks.
> 
> > > +static int migration_completion_postcopy(MigrationState *s) {
> > > +    trace_migration_completion_postcopy_end();
> > > +
> > > +    qemu_mutex_lock_iothread();
> > > +    qemu_savevm_state_complete_postcopy(s->to_dst_file);
> > > +    qemu_mutex_unlock_iothread();
> > > +
> > > +    /*
> > > +     * Shutdown the postcopy fast path thread.  This is only needed when
> > dest
> > > +     * QEMU binary is old (7.1/7.2).  QEMU 8.0+ doesn't need this.
> > > +     */
> > > +    if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
> > > +        postcopy_preempt_shutdown_file(s);
> > > +    }
> > > +
> > > +    trace_migration_completion_postcopy_end_after_complete();
> > > +
> > > +    return 0;
> > 
> > Always return 0?  Make it void.
> 
> OK.
> 
> > > +static void migration_completion(MigrationState *s) {
> > > +    int ret = -1;
> > > +    int current_active_state = s->state;
> > > +
> > > +    if (s->state == MIGRATION_STATUS_ACTIVE) {
> > > +        ret = migration_completion_precopy(s, &current_active_state);
> > > +    } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> > > +        ret = migration_completion_postcopy(s);
> > 
> > Here we can set ret = 0.
> 
> Yes, after migration_completion_postcopy is made "void".

Looks good to me, after addressing Isaku's comments.

The current_active_state is very unfortunate, along with most of the calls
to migrate_set_state() - I bet most of the code will definitely go wrong if
that cmpxchg didn't succeed inside of migrate_set_state(), IOW in most
cases we simply always want:

  migrate_set_state(&s->state, s->state, XXX);

Not sure whether one pre-requisite patch is good to have so we can rename
migrate_set_state() to something like __migrate_set_state(), then:

  migrate_set_state(s, XXX) {
    __migrate_set_state(&s->state, s->state, XXX);
  }

I don't even know whether there's any call site that will need
__migrate_set_state() for real..

Thanks,

-- 
Peter Xu