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

[Wei Wang <wei.w.wang@intel.com>]

On 11/28/2018 05:32 PM, Peter Xu wrote:
> So what I am worrying here are corner cases where we might forget to
> stop the hinting.  I'm fabricating one example sequence of events:
>
>    (start migration)
>    START_MIGRATION
>    BEFORE_SYNC
>    AFTER_SYNC
>    ...
>    BEFORE_SYNC
>    AFTER_SYNC
>    (some SaveStateEntry failed rather than RAM, then
>     migration_detect_error returned MIG_THR_ERR_FATAL so we need to
>     fail the migration, however when running the previous
>     ram_save_iterate for RAM's specific SaveStateEntry we didn't see
>     any error so no ERROR event detected)
>
> Then it seems the hinting will last forever.  Considering that now I'm
> not sure whether this can be done ram-only, since even if you capture
> ram_save_complete() and at the same time you introduce PRECOPY_END you
> may still miss the PRECOPY_END event since AFAIU ram_save_complete()
> won't be called at all in this case.
>
> Could this happen?

Thanks, indeed this case could happen if we add PRECOPY_END in
ram_save_complete.

How about putting PRECOPY_END in ram_save_cleanup?
I think it would be called in any case.

I'm also thinking probably we don't need PRECOPY_ERR when we have 
PRECOPY_END,
and what do you think of the notifier names below:

+typedef enum PrecopyNotifyReason {
+    PRECOPY_NOTIFY_RAM_SAVE_END = 0,
+    PRECOPY_NOTIFY_RAM_SAVE_START = 1,
+    PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
+    PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3,
+    PRECOPY_NOTIFY_RAM_SAVE_MAX = 4,
+} PrecopyNotifyReason;


> > > [1]
> > >
> > > > > Another thing to mention about the "reasons" (though I see it more
> > > > > like "events"): have you thought about adding a PRECOPY_NOTIFY_END?
> > > > > It might help in some cases:
> > > > >
> > > > >      - then you don't need to trickily export the migrate_postcopy()
> > > > >        since you'll notify that before postcopy starts
> > > > I'm thinking probably we don't need to export migrate_postcopy even now.
> > > > It's more like a sanity check, and not needed because now we have the
> > > > notifier registered to the precopy specific callchain, which has ensured
> > > > that
> > > > it is invoked via precopy.
> > > But postcopy will always start with precopy, no?
> > Yes, but I think we could add the check in precopy_notify()
> I'm not sure that's good.  If the notifier could potentially have
> other user, they might still work with postcopy, and they might expect
> e.g. BEFORE_SYNC to be called for every sync, even if it's at the
> precopy stage of a postcopy.

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.



> In that sense I still feel the
> PRECOPY_END is better (so contantly call it at the end of precopy, no
> matter whether there's another postcopy afterwards).  It sounds like a
> cleaner interface.

Probably I still haven't got the point how PRECOPY_END could help above yet.

Best,
Wei