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

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

On 11/29/2018 01:10 PM, Peter Xu wrote:
> On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote:
> > On 11/28/2018 05:32 PM, Peter Xu wrote:
> 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...
>
> IMHO the problem is not that complicated.  How about this proposal:
>
> [1]
>
>    typedef enum PrecopyNotifyReason {
>      PRECOPY_NOTIFY_RAM_START,
>      PRECOPY_NOTIFY_RAM_BEFORE_SYNC,
>      PRECOPY_NOTIFY_RAM_AFTER_SYNC,
>      PRECOPY_NOTIFY_COMPLETE,
>      PRECOPY_NOTIFY_RAM_CLEANUP,
>    };
>
> The first three keep the same as your old ones.  Notify RAM_CLEANUP in
> ram_save_cleanup() to make sure it'll always be cleaned up (the same
> as PRECOPY_END, just another name).  Notify COMPLETE in
> qemu_savevm_state_complete_precopy() to show that precopy is
> completed.  Meanwhile on balloon side you should stop the hinting for
> either RAM_CLEANUP or COMPLETE event.  Then either:
>
>    - precopy is switching to postcopy, or
>    - precopy completed, or
>    - precopy failed/cancelled
>
> You should always get at least a notification to stop the balloon.
> Though you could also get one RAM_CLEANUP after one COMPLETE, but
> the balloon should easily handle it (stop the hinting twice).

Sounds better, thanks.


> Here maybe you can even remove the "RAM_" in both RAM_START and
> RAM_CLEANUP if we're going to have COMPLETE since after all it'll be
> not only limited to RAM.
>
> Another suggestion is that you can add an Error into the notify hooks,
> please refer to the postcopy one:
>
>    int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp);
>
> So the hook functions have a way to even stop the migration (though
> for balloon hinting it'll be always optional so no error should be
> reported...), then the two interfaces are matched.

Yes, I removed that "errp" as it's not needed by the balloon usage.
But no problem, I can add it back if no objections from others.

Best,
Wei