Re: [PATCH v2 06/29] migration: Add auto-pause capability

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Add a capability that allows the management layer to delegate to QEMU
>>> the decision of whether to pause a VM and perform a non-live
>>> migration. Depending on the type of migration being performed, this
>>> could bring performance benefits.
>>>
>>> Note that the capability is enabled by default but at this moment no
>>> migration scheme is making use of it.
>>
>> This sounds as if the capability has no effect unless the "migration
>> scheme" (whatever that may be) opts into using it.  Am I confused?
>>
>
> What I mean here is that this capability is implemented and functional,
> but I'm not retroactively enabling any existing migration code to use
> auto-pause. Otherwise people would start seeing their guests being
> paused before migraton in scenarios they never used to pause.
>
> By "migration scheme" I mean types of migration. Or modes of
> operation. Or exclusive parameters. Anything that is different enough
> from what exists today that we would consider a different type of
> migration. Anything that allow us to break backward compatibility
> (because it never existed before to begin with).
>
> E.g. this series introduces the fixed-ram migration. That never existed
> before. So from the moment we enable that code to use this capability,
> it will always do auto-pause, unless the management layer wants to avoid
> it.

So the auto-pause's *effective* default depends on the migration scheme:
certain new schemes pause by default, everything else doesn't.  Is this
a good idea?

If it is, then we need to document this behavior clearly.

Here's another way to design the interface: keep the default behavior
consistent (no pause), and provide a way to ask for pause (fails if
migration scheme doesn't support it).

>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>
>> [...]
>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index db3df12d6c..74f12adc0e 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -523,6 +523,10 @@
>>>  #     and can result in more stable read performance.  Requires KVM
>>>  #     with accelerator property "dirty-ring-size" set.  (Since 8.1)
>>>  #
>>> +# @auto-pause: If enabled, allows QEMU to decide whether to pause the
>>> +#     VM before migration for an optimal migration performance.
>>> +#     Enabled by default. (since 8.1)
>>
>> If this needs an opt-in to take effect, it should be documented.
>
> Someting like this perhaps?
>
> # @auto-pause: If enabled, allows QEMU to decide whether to pause the VM
> #     before migration for an optimal migration performance. Enabled by
> #     default. New migration code needs to opt-in at
> #     migration_should_pause(), otherwise this behaves as if
> #     disabled. (since 8.2)

Remember, this is user-facing documentation.  Talking about
migration_should_pause() makes no sense there.

Instead, you need to document what @auto-pause does: pause when a
condition specific to the migration scheme is met, and specify the
condition for each migration scheme.  The condition could be "never"
(auto-pause has no effect), "always", or something in between.

A configuration knob that has no effect feels like an interface blemish.

>>> +#
>>>  # Features:
>>>  #
>>>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>>> @@ -539,7 +543,7 @@
>>>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>>>             'validate-uuid', 'background-snapshot',
>>>             'zero-copy-send', 'postcopy-preempt', 'switchover-ack',
>>> -           'dirty-limit'] }
>>> +           'dirty-limit', 'auto-pause'] }
>>>  
>>>  ##
>>>  # @MigrationCapabilityStatus: