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 01:26 PM, Peter Xu wrote:
> Ok thanks.  Please just make sure you will capture all the error
> cases, e.g., I also see path like this (a few lines below):
>
>          if (pages < 0) {
>              qemu_file_set_error(f, pages);
>              break;
>          }
>
> It seems that you missed that one.

I think that one should be fine. This notification is actually put at the
bottom of ram_save_iterate. All the above error will bail out to the "out:"
path and then go to call precopy_notify(PRECOPY_NOTIFY_ERR).

> I would even suggest that you capture the error with higher level.
> E.g., in migration_iteration_run() after qemu_savevm_state_iterate().
> Or we can just check the return value of qemu_savevm_state_iterate(),
> which we have had ignored so far.

Not very sure about the higher level, because other SaveStateEntry may cause
errors that this feature don't need to care, I think we may only need it 
in ram_save.


> [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()


> > >     - you'll have a solid point that you'll 100% guarantee that we'll
> > >       stop the free page hinting and don't need to worry about whether
> > >       there is chance the hinting will be running without an end [2].
> > Thanks, I think it makes sense. Plan to add PRECOPY_NOTIFY_END in
> > ram_save_complete.
> Yeah you can.
>
> Btw, if you're mostly adding the notifies only in RAM-only codes, then
> you can consider add the "RAM" into the names of events too to be
> clear.

Sounds good, will try and see if the name would become too long :)

> My suggestion at [1] is precopy general, but you can still capture it
> at the end of ram_save_iterate, then they are RAM-only again.  Please
> feel free to choose what fits more...

OK, thanks.

Best,
Wei