Re: [PATCH V2 13/13] migration: cpr-transfer mode

[Steven Sistare <steven.sistare@oracle.com>]

On 10/8/2024 3:11 PM, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
>> On Tue, Oct 08, 2024 at 03:28:30PM -0300, Fabiano Rosas wrote:
>>>>>> +    /* Close cpr socket to tell source that we are listening */
>>>>>> +    cpr_state_close();
>>>>>
>>>>> Would it be possible to use some explicit reply message to mark this?
>>>>
>>>> In theory yes, but I fear that using a return channel with message parsing and
>>>> dispatch adds more code than it is worth.
>>>
>>> I think this approach is fine for now, but I wonder whether we could
>>> reuse the current return path (RP) by starting it earlier and take
>>> benefit from it already having the message passing infrastructure in
>>> place. I'm actually looking ahead to the migration handshake thread[1],
>>> which could be thought to have some similarity with the early cpr
>>> channel. So having a generic channel in place early on to handle
>>> handshake, CPR, RP, etc. could be a good idea.
>>
>> The current design relies on CPR stage happens before device realize()s, so
>> I assume migration channel (including RP) isn't easily applicable at as
>> early as this stage.
> 
> Well, what is the dependency for the RP? If we can send CPR state, we
> can send QEMU_VM_COMMAND, no?

The CPR state channel is (and must be) used before migration_object_init,
and before the normal migration channel is opened.  Thus we cannot use the
normal return path.

- Steve

>> However I think dest qemu can directly write back to the cpr_uri channel
>> instead if we want and then follow a protocol simple enough (even though
>> it'll be separate from the migration stream protocol).
>>
>> What worries me more (besides using HUP as of now..) is cpr_state_save() is
>> currently synchronous and can block the main iothread.  It means if cpr
>> destination is not properly setup, it can hang the main thread (including
>> e.g. QMP monitor) at qio_channel_socket_connect_sync().  Ideally we
>> shouldn't block the main thread.
>>
>> If async-mode can be done, it might be even easier, e.g. if we do
>> cpr_state_save() in a thread, after qemu_put*() we can directly qemu_get*()
>> in the same context with the pairing return qemufile.
>>
>> But maybe we can do it in two steps, merging HUP first.  Then when a better
>> protocol (plus async mode) ready, one can boost QEMU_CPR_FILE_VERSION.
>> I'll see how Steve wants to address it.
> 
> I agree HUP is fine at the moment.
> 
>>
>>>
>>> Anyway, I'm probing on this a bit so I can start drafting something. I
>>> got surprised that we don't even have the capability bits in the stream
>>> in a useful way (currently, configuration_validate_capabilities() does
>>> kind of nothing).
>>>
>>> 1- https://wiki.qemu.org/ToDo/LiveMigration#Migration_handshake
>>
>> Happy to know this. I was thinking whether I should work on this even
>> earlier, so if you're looking at that it'll be great.
> 
> As of half an hour ago =) We could put a feature branch up and work
> together, if you have more concrete thoughts on how this would look like
> let me know.
> 
>>
>> The major pain to me is the channel establishment part where we now have
>> all kinds of channels, so we should really fix that sooner (e.g., we hope
>> to enable multifd + postcopy very soon, that requires multifd and preempt
>> channels appear in the same time).  It was reasonable the vfio/multifd
>> series tried to fix it.
>