On Fri, Nov 30, 2018 at 01:05:51PM +0800, Wei Wang wrote:
> On 11/29/2018 01:10 PM, Peter Xu wrote:
> > On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote:
> > I think this precopy notifier callchain is expected to be used only for
> > the precopy mode. Postcopy has its dedicated notifier callchain that
> > users could use.
> >
> > How about changing the migrate_postcopy() check to "ms->start_postcopy":
> >
> > bool migration_postcopy_start(void)
> > {
> > MigrationState *s;
> >
> > s = migrate_get_current();
> >
> > return atomic_read(&s->start_postcopy);
> > }
> >
> >
> > static void precopy_notify(PrecopyNotifyReason reason)
> > {
> > if (migration_postcopy_start())
> > return;
> >
> > notifier_list_notify(&precopy_notifier_list, &reason);
> > }
> >
> > If postcopy started with precopy, the precopy optimization feature
> > could still be used until it switches to the postcopy mode.
> > I'm not sure we can use start_postcopy. It's a variable being set in
> > the QMP handler but it does not mean postcopy has started. I'm afraid
> > there can be race where it's still precopy but the variable is set so
> > event could be missed...
>
> Peter,
> I just found that migration_bitmap_sync is also called by
> ram_postcopy_send_discard_bitmap().
> But we don't expect notifier_list_notify to be called in the postcopy mode.
>
> So, probably we still need the start_postcopy check in notifier_list_notify
> though we have
> the COMPLETE notifier.
I still don't think using start_postcopy is a good idea, as
explained in my previous reply.
Maybe move the notify()s outside migration_bitmap_sync() but only at
the call sites in precopy? Say these three:
ram_init_bitmaps[3508] migration_bitmap_sync(rs);
ram_save_complete[3731] migration_bitmap_sync(rs);
ram_save_pending[3776] migration_bitmap_sync(rs);
Or you can introduce migration_bitmap_sync_precopy() and let precopy
code call that one instead.
PS. I'm a bit unsure on why we need to sync bitmap in ram_init_bitmaps.
I feel like it can be removed...
Regards,
--
Peter Xu
