Re: [PATCH V4 10/14] migration: stop vm for cpr

["Cédric Le Goater" <clg@redhat.com>]

On 3/1/24 02:28, Peter Xu wrote:
> On Thu, Feb 29, 2024 at 10:21:14AM -0500, Steven Sistare wrote:
>> On 2/25/2024 9:08 PM, Peter Xu wrote:
>>> On Thu, Feb 22, 2024 at 09:28:36AM -0800, Steve Sistare wrote:
>>>> When migration for cpr is initiated, stop the vm and set state
>>>> RUN_STATE_FINISH_MIGRATE before ram is saved.  This eliminates the
>>>> possibility of ram and device state being out of sync, and guarantees
>>>> that a guest in the suspended state remains suspended, because qmp_cont
>>>> rejects a cont command in the RUN_STATE_FINISH_MIGRATE state.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>
>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>>
>>> cpr-reboot mode keeps changing behavior.
>>>
>>> Could we declare it "experimental" until it's solid?  Maybe a patch to
>>> document this?
>>>
>>> Normally IMHO we shouldn't merge a feature if it's not complete, however
>>> cpr-reboot is so special that the mode itself is already merged in 8.2
>>> before I started to merge patches, and it keeps changing things.  I don't
>>> know what else we can do here besides declaring it experimental and not
>>> declare it a stable feature.
>>
>> Hi Peter, the planned/committed functionality for cpr-reboot changed only once, in:
>>      migration: stop vm for cpr
>>
>> Suspension to support vfio is an enhancement which adds to the basic functionality,
>> it does not change it.  This was planned all along, but submitted as a separate
> 
> If VFIO used to migrate without suspension and now it won't, it's a
> behavior change?
> 
>> series to manage complexity, as I outlined in my qemu community presentation,
>> which I emailed you at the time.
>>
>> Other "changes" that arose during review were just clarifications and explanations.
>>
>> So, I don't think cpr-reboot deserves to be condemned to experimental limbo.
> 
> IMHO it's not about a feature being condemned, it's about a kindful
> heads-up to the users that one needs to take risk on using it until it
> becomes stable, it also makes developers easier because of no limitation on
> behavior change.  If all the changes are landing, then there's no need for
> such a patch.
> 
> If so, please propose the planned complete document patch. I had a feeling
> that cpr is still not fully understood by even many developers on the list.
> It'll be great that such document will contain all the details one needs to
> know on the feature, etc. meaning of the name cpr-reboot (what is "cpr"),
> when to use it, how to use it, how it differs from "normal" mode
> (etc. lifted limitations on some devices that used to block migration?),
> what is enforced (vm stop, suspension, etc.) and what is optionally offered
> (VFIO, shared-mem, etc.), the expected behaviors, etc.
> 
> When send it, please copy relevant people (Alex & Cedric for VFIO, while
> Markus could also be a good candidate considering the QMP involvement).

I second that. If we could have a file under docs/, it would be great.

Thanks,

C.