Re: [PATCH v20 13/15] net/tap: support local migration with virtio-net

On 29.07.26 14:30, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <[email protected]> writes:
> 
>> Support transferring of TAP state (including open fd).
>>
>> Add new property "x-local-migration-supported", which defines
>> whether local-migration is actually supported for this TAP device.
>> Starting from 11.2 QEMU Machine Types it's enabled by default.
>>
>> Note that local-migration is enabled by global "local" migration
>> parameter, but individual devices may have additional options to
>> enable/disable it per device.
>>
>> The tricky thing is that we need to know whether to call open/connect in
>> TAP initialization code, i.e. we need to know the value of migration
>> parameter "local" when creating the TAP device.  For incoming migration,
>> we can know only for TAP devices created with QMP after setting the
>> migration parameter with QMP.
>>
>> So the full picture is:
>>
>> On source, to start outgoing "local" migration you need:
>>
>>   - migration parameter "local" set to true
>>   - "x-local-migration-supported" TAP option set to true (the
>>     default, starting from 11.2 QEMU Machine Types)
> 
> Is the machine type part still accurate?  The description in the QAPI
> schema has (default: false, since 11.2).

Oops, need to update commit message.

> 
>>
>> If at least one of these options is not set, TAP backend
>> doesn't participate in migration.
>>
>> On target, things are more difficult:
>>
>> Same, you need both "local" and "x-local-migration-supported"
>> be set. And same, if one of them is not set, TAP backend
>> is initialized as usual, and doesn't accept any incoming
>> state.
>>
>> Additionally, if you are going to set "local", it must be
>> set before creating the TAP device. If TAP device created
>> with "local" unset, it initializes as usual. If you enable
>> "local" after it and start incoming migration, it will fail
>> in .pre_load handler of TAP backend.
>>
>> Moreover, there are interface restrictions: if you create TAP
>> device when QEMU is in INCOMING state, and both "local"
>> and "x-local-migration-supported" set, most of TAP options are
>> not allowed, and script/downscript are required to be explicitly
>> unset (set to "" or "no").
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
>> Reviewed-by: Ben Chaney <[email protected]>
> 
> [...]
> 
>> diff --git a/qapi/net.json b/qapi/net.json
>> index acb8594c952..e6921912e12 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -437,6 +437,29 @@
>>   # @poll-us: maximum number of microseconds that could be spent on busy
>>   #     polling for tap (since 2.7)
>>   #
>> +# @x-local-migration-supported: enable local migration for this TAP
>> +#     backend.  When set, local migration is enabled/disabled by
>> +#     migration parameter @local for this TAP backend.  When unset,
>> +#     migration parameter @local is ignored for this TAP backend.
>> +#     To be able to do incoming local migration of a TAP backend,
>> +#     migration parameter @local must be set _before_ creating the
>> +#     TAP backend.  Otherwise, TAP backend is initialized as usual,
>> +#     opening/creating TAP devices in kernel.  In this case further
>> +#     local incoming migration (with migration parameter @local set
>> +#     after creating TAP backend with @x-local-migration-supporeted
>> +#     parameter set) will simply fail.
> 
> Either have a a blank line here so you actually get two paragraphs, or
> refill the entire description to avoid the illusion of two paragraphs.
> 
>> +#     Moreover, when QEMU is in incoming migration state, migration
>> +#     parameter @local is set and @x-local-migration-supported is set,
>> +#     the following options are not supported and must not be set:
>> +#     @fd, @fds, @helper, @br, @ifname, @sndbuf, @vnet_hdr.
>> +#     Additionally in this case @script and @downscipt must be
> 
> @downscript
> 
>> +#     explicitly disabled (empty strings or "no").
> 
> "no" is deprecated [PATCH 3].  We'll have to remember deleting 'or "no"'
> here when remove it.  Easy to forget.  Delete it now?
> 
> Maybe
> 
>     #     Additionally, @script and @downscript must be explicitly disabled
>     #     then.


Agree.

> 
>> +#     (default: false, since 11.2)
>> +#
>> +# Features:
>> +#
>> +# @unstable: Member @x-local-migration-supported is experimental.
>> +#
>>   # Since: 1.2
>>   ##
>>   { 'struct': 'NetdevTapOptions',
>> @@ -455,7 +478,9 @@
>>       '*vhostfds':   'str',
>>       '*vhostforce': 'bool',
>>       '*queues':     'uint32',
>> -    '*poll-us':    'uint32'} }
>> +    '*poll-us':    'uint32',
>> +    '*x-local-migration-supported': {
>> +      'type': 'bool', 'features' : [ 'unstable'] } } }
>>   
>>   ##
>>   # @NetdevSocketOptions:
> 
> Naming is hard...
> 
> "Supported" sounds like a property of the QEMU process.  That's not what
> this is.  It's an on/off switch that happens to be in series with
> another on/off switch, namely migration parameter @local.
> 
> Maybe
> 
>      @permit-local-migration: permit local migration for this TAP
>          backend.  When set, local migration is enabled/disabled by
>          migration parameter @local for this TAP backend.  ...
> 

Good for me. Will use, if no more opinions.

Or x-permit-local-migration? I've added "x-" prefix as Michael suggested.
And it seems reasonable, as it's not only QAPI field, but also QOM property,
and we don't have "unstable" features for properties.

-- 
Best regards,
Vladimir