OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

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

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);

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.


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
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
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

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.


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]