Re: [PATCH V5 15/23] migration: cpr-transfer mode

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

On 1/28/2025 6:56 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
>> On 1/17/2025 8:44 AM, Markus Armbruster wrote:
>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>
>>>> On 1/7/2025 7:05 AM, Markus Armbruster wrote:
>>>>> Steve Sistare <steven.sistare@oracle.com> writes:
>>>>>
>>>>>> Add the cpr-transfer migration mode, which allows the user to transfer
>>>>>> a guest to a new QEMU instance on the same host with minimal guest pause
>>>>>> time, by preserving guest RAM in place, albeit with new virtual addresses
>>>>>> in new QEMU, and by preserving device file descriptors.  Pages that were
>>>>>> locked in memory for DMA in old QEMU remain locked in new QEMU, because the
>>>>>> descriptor of the device that locked them remains open.
>>>>>>
>>>>>> cpr-transfer preserves memory and devices descriptors by sending them to
>>>>>> new QEMU over a unix domain socket using SCM_RIGHTS.  Such CPR state cannot
>>>>>> be sent over the normal migration channel, because devices and backends
>>>>>> are created prior to reading the channel,
>>>>>
>>>>> Is that an artifact of the way QEMU starts up, or is it fundamental?
>>>>
>>>> Hi Markus, welcome back and Happy New Year!
>>>
>>> Thank you!  A late happy new year for you, too!
>>
>> Sorry for the delay, I have been heads down preparing a new series.
> 
> No sweat :)
> 
>>>> This is a deeply ingrained artifact.  Changing it would require radically
>>>> refactoring the information passed on the command line vs the information
>>>> passed via monitor.
>>>
>>> More on this below.
>>>
>>>>>>                                               so this mode sends CPR state
>>>>>> over a second "cpr" migration channel.  New QEMU reads the cpr channel
>>>>>> prior to creating devices or backends.  The user specifies the cpr channel
>>>>>> in the channel arguments on the outgoing side, and in a second -incoming
>>>>>> command-line parameter on the incoming side.
>>>>>>
>>>>>> The user must start old QEMU with the the '-machine aux-ram-share=on' option,
>>>>>> which allows anonymous memory to be transferred in place to the new process
>>>>>> by transferring a memory descriptor for each ram block.  Memory-backend
>>>>>> objects must have the share=on attribute, but memory-backend-epc is not
>>>>>> supported.
>>>>>>
>>>>>> The user starts new QEMU on the same host as old QEMU, with command-line
>>>>>> arguments to create the same machine, plus the -incoming option for the
>>>>>> main migration channel, like normal live migration.  In addition, the user
>>>>>> adds a second -incoming option with channel type "cpr".  The CPR channel
>>>>>> address must be a type, such as unix socket, that supports SCM_RIGHTS.
>>>>>>
>>>>>> To initiate CPR, the user issues a migrate command to old QEMU, adding
>>>>>> a second migration channel of type "cpr" in the channels argument.
>>>>>> Old QEMU stops the VM, saves state to the migration channels, and enters
>>>>>> the postmigrate state.  New QEMU mmap's memory descriptors, and execution
>>>>>> resumes.
>>>>>>
>>>>>> The implementation splits qmp_migrate into start and finish functions.
>>>>>> Start sends CPR state to new QEMU, which responds by closing the CPR
>>>>>> channel.  Old QEMU detects the HUP then calls finish, which connects the
>>>>>> main migration channel.
>>>>>>
>>>>>> In summary, the usage is:
>>>>>>
>>>>>>      qemu-system-$arch -machine aux-ram-share=on ...
>>>>>>
>>>>>>      start new QEMU with "-incoming <main-uri> -incoming <cpr-channel>"
>>>>>>
>>>>>>      Issue commands to old QEMU:
>>>>>>        migrate_set_parameter mode cpr-transfer
>>>>>>
>>>>>>        {"execute": "migrate", ...
>>>>>>            {"channel-type": "main"...}, {"channel-type": "cpr"...} ... }
>>>>>>
>>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>> [...]
>>>>>
>>>>>> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
>>>>>> index f31deb3..2210f0c 100644
>>>>>> --- a/migration/vmstate-types.c
>>>>>> +++ b/migration/vmstate-types.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>     #include "qemu-file.h"
>>>>>>     #include "migration.h"
>>>>>>     #include "migration/vmstate.h"
>>>>>> +#include "migration/client-options.h"
>>>>>>     #include "qemu/error-report.h"
>>>>>>     #include "qemu/queue.h"
>>>>>>     #include "trace.h"
>>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>>> index a605dc2..35309dc 100644
>>>>>> --- a/qapi/migration.json
>>>>>> +++ b/qapi/migration.json
>>>>>> @@ -614,9 +614,47 @@
>>>>>>     #     or COLO.
>>>>>>     #
>>>>>>     #     (since 8.2)
>>>>>> +#
>>>>>> +# @cpr-transfer: This mode allows the user to transfer a guest to a
>>>>>> +#     new QEMU instance on the same host with minimal guest pause
>>>>>> +#     time, by preserving guest RAM in place, albeit with new virtual
>>>>>> +#     addresses in new QEMU.  Devices and their pinned pages will also
>>>>>> +#     be preserved in a future QEMU release.
>>>>>
>>>>> Maybe "@cpr-transfer: Checkpoint and restart migration mode minimizes
>>>>> guest pause time by transferring guest RAM without copying it."
>>>>
>>>> "Checkpoint and restart migration mode" is ambiguous.  It would be
>>>> "Checkpoint and restart transfer migration mode".  That's a mouthful!
>>>> "This mode" is unambiguous, and matches the concise style of describing
>>>> options in this file.  Few if any of the options in this file repeat the
>>>> name of the option in the description.
>>>
>>> True.  But will readers understand what "CPR" stands for?  Do they need
>>> to understand?
>>
>> No, IMO they do not need to know the full spelling of the acronym to use the
>> functionality.  It is spelled out in a few places now.  It could be spelled
>> out in more places in the future.
> 
> Alright.
> 
>>>>> If you want to mention the guest RAM mapping differs between old and new
>>>>> QEMU, that's fine, but it's also detail, so I'd do it further down.
>>>>
>>>> I'll strike it.  I agree the user does not need to know.
>>>>
>>>>>> +#
>>>>>> +#     The user starts new QEMU on the same host as old QEMU, with
>>>>>> +#     command-line arguments to create the same machine, plus the
>>>>>> +#     -incoming option for the main migration channel, like normal
>>>>>> +#     live migration.  In addition, the user adds a second -incoming
>>>>>> +#     option with channel type "cpr".  The CPR channel address must
>>>>>> +#     be a type, such as unix socket, that supports SCM_RIGHTS.
>>>>>
>>>>> Permit me to indulge in a bit of pedantry...  A channel address doesn't
>>>>> support SCM_RIGHTS, only a channel may.  A channel supports it when it
>>>>> is backed by a UNIX domain socket.  The channel's socket's transport
>>>>> type need not be 'unix' for that, it could also be 'fd'.
>>>>>
>>>>> Suggest something like "This CPR channel must be a UNIX domain socket."
>>>>>
>>>>> If you want to say why, that's fine: "This CPR channel must support file
>>>>> descriptor transfer, i.e. it must be a UNIX domain socket."
>>>>>
>>>>> If you want to mention SCM_RIGHTS, that's fine, too: "This CPR channel
>>>>> must support file descriptor transfer with SCM_RIGHTS, i.e. it must be a
>>>>> UNIX domain socket."
>>>>
>>>> OK.
>>>>
>>>>>> +#
>>>>>> +#     To initiate CPR, the user issues a migrate command to old QEMU,
>>>>>> +#     adding a second migration channel of type "cpr" in the channels
>>>>>
>>>>> in the channel's
>>>>>
>>>>>> +#     argument.  Old QEMU stops the VM, saves state to the migration
>>>>>> +#     channels, and enters the postmigrate state.  Execution resumes
>>>>>> +#     in new QEMU.
>>>>>> +#
>>>>>> +#     New QEMU reads the CPR channel before opening a monitor, hence
>>>>>> +#     the CPR channel cannot be specified in the list of channels for
>>>>>> +#     a migrate-incoming command.  It may only be specified on the
>>>>>> +#     command line.
>>>>>
>>>>> This is a restriction that could conceivably be lifted in the future,
>>>>> right?
>>>>
>>>> Yes, but lifting it requires the big command-line vs monitor restructuring
>>>> I mentioned earlier.  IMO that is far enough in the future (and possibly never)
>>>> that adding a "Currently" disclaimer would be deceptive.
>>>
>>> Now I'm confused.
>>>
>>> Earlier, you explained why we can't simply send CPR state via the normal
>>> migration channel: we create devices and backends much earlier long
>>> before we receive from the migration channel.  Correct?
>>
>> Correct.
>>
>>> Here, you're documenting that the CPR channel can only be specified on
>>> the command line, not with migrate-incoming.  Isn't that a different
>>> problem?
>>
>> They are entangled problems.
>>
>> * cpr state must loaded before backends are created
> 
> I understand this is fundamental, i.e. CPR state is *required* to create
> backends.  Correct?

Correct.

>> * monitor must be created after backends are created
>>     (because the chardevs that define a monitor are backends).
> 
> This is an artifact of the way we configure monitors (use of a character
> backend) and create character backends (all at once at a certain point
> in startup).
> 
> Some management applications would prefer to use the command line just
> for setting up QMP, and do everything else in QMP.  Understandable!
> They need to use QMP anyway, and mostly eliminating the command line
> would simplify things.  Moreover, only QMP provides introspection.
> 
> To enable this, we need to provide a way to start a QMP monitor early
> enough to set up everything else.  If we pull this off, "monitor must be
> created after backends are created" is no longer true.

Yes.  It's a big change, which is why I ventured "unlikely to change any
time soon".

> Aside: in my view, QMP and command line are transports wrapping around
> an abstract interface.  I'd like to have them wrap around the *same*
> abstract interface.
> 
>> * migrate-incoming must come after the monitor is created
> 
> Fundamental.
> 
>> ==> migrate-incoming cannot specify the cpr-state channel
>>
>> This restriction is unlikely to change any time soon, if ever.
> 
> I wouldn't bet my own money on this :)

OK, "if ever" is too pessimistic.

>> I have documented it as is, without speculating about future state.
>> If users do not like it they can request changes.
> 
> I'm not objecting to your doc text.  I asked to better understand your
> thinking, because I needed that for a competent review.
> 
>>>>> What happens if a user tries to specify it with migrate-incoming?  Fails
>>>>> cleanly?  What's the error message?
>>>>
>>>> It fails cleanly with a pre-existing error message that could be more
>>>> descriptive, so I will improve it, thanks.
>>>>
>>>>> Maybe simply:
>>>>>
>>>>>             Currently, the CPR channel can only be specified on the command
>>>>>             line, not with the migrate-incoming command.
>>>>>
>>>>> with a big, fat comment explaining the restriction next to the spot
>>>>> that reports the error.
>>>>>
>>>>>> +#
>>>>>> +#     The main channel address cannot be a file type, and for the tcp
>>>>>> +#     type, the port cannot be 0 (meaning dynamically choose a port).
>>>>>
>>>>> What's "the tcp type"?  URI "tcp:..." / channel
>>>>> addr.transport=socket,addr.type=inet?
>>>>
>>>> I will clarify.
>>>>
>>>>>> +#
>>>>>> +#     Memory-backend objects must have the share=on attribute, but
>>>>>> +#     memory-backend-epc is not supported.  The VM must be started
>>>>>> +#     with the '-machine aux-ram-share=on' option.
>>>>>> +#
>>>>>> +#     When using -incoming defer, you must issue the migrate command
>>>>>> +#     to old QEMU before issuing any monitor commands to new QEMU.
>>>>>> +#     However, new QEMU does not open and read the migration stream
>>>>>> +#     until you issue the migrate incoming command.
>>>>>
>>>>> I think some (all?) instances of "old QEMU" and "new QEMU" would read
>>>>> better as "the old QEMU" and "the new QEMU".
>>>>
>>>> Maybe slightly,
>>>
>>> A second opinion from a native speaker would be nice.
>>
>> I have appreciated all your feedback and made many changes, and it has made
>> the code and documentation better.  Thanks for that. But right now, the V9
>> patches are queued, pass CI, and are ready to roll. I would hope at this point
>> that no one would consider "old QEMU" vs "the old QEMU" to be a showstopper
>> that requires new patches to be posted.
> 
> Certainly not a blocker.  If we decide it's an improvement we want, we
> can easily do it on top.
> 
> I routinely point out lots of little things that aren't blockers.  I try
> to be clear what is a blocker and what isn't, but I'm far from perfect
> there.  Thanks for your patience!

Thanks.  Sounds like we're good, and Fabiano can proceed with integration.

- Steve