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, ¤t_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
