Re: [Qemu-devel] [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