Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy

[Peter Xu <peterx@redhat.com>]

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