Re: [PATCH v5 16/19] qapi: add interface for local TAP migration

[Fabiano Rosas <farosas@suse.de>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 19.09.25 18:20, Fabiano Rosas wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>> Hi Vladimir, as usual with "qapi:" patches, some comments about
>> language:
>> 
>>> To migrate virtio-net TAP device backend (including open fds) locally,
>>> user should simply set migration parameter
>>>
>>>     fds = [virtio-net]
>>>
>>> Why not simple boolean? To simplify migration to further versions,
>>> when more devices will support fds migration.
>>>
>>> Alternatively, we may add per-device option to disable fds-migration,
>>> but still:
>>>
>>> 1. It's more comfortable to set same capabilities/parameters on both
>>> source and target QEMU, than care about each device.
>>>
>>> 2. To not break the design, that machine-type + device options +
>>> migration capabilites and parameters are fully define the resulting
>>> migration stream. We'll break this if add in future more fds-passing
>>> support in devices under same fds=true parameter.
>>>
>> 
>> These arguments look convincing to me.
>> 
>
> [..]
>
>>>       return true;
>>>   }
>>>   
>>> @@ -1297,6 +1315,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>>>       if (params->has_direct_io) {
>>>           dest->direct_io = params->direct_io;
>>>       }
>>> +
>>> +    if (params->has_fds) {
>>> +        dest->has_fds = true;
>> 
>> I think what you want is to set this in
>> migrate_params_init(). block_bitmap_mapping is a bit of an outlier in
>> that it takes an empty *input* as valid.
>
> Hmm I made it behave like block_bitmap_mapping because it also a list.
>
> As I understand, for list we have to treat empty list not like absent parameter: we should have a way
> to clear previously set list by subsequent migrate-set-parameters call.
>

Sorry, I explained myself poorly. Yes, the empty list is valid and it
clears a previously set value. But that's just:

migrate_params_init(MigrationParameters *params):
    ...
    params->has_fds = true;

migrate_params_test_apply(MigrateSetParameters *params,
                          MigrationParameters *dest):
    ...
    /* means user provided it */                          
    if (params->has_fds) {
        dest->fds = params->fds;
    }

migrate_params_check(MigrationParameters *params):
    ...
    if (params->has_fds) {
       /* empty list ok */
    }

migrate_params_apply(MigrateSetParameters *params):
    ...
    if (params->has_fds) {
        qapi_free_...(s->parameters.fds);
        /* clones the full list or the empty list, no difference */
        s->parameters.fds = QAPI_CLONE(..., params->fds);
    }

The block_bitmap_mapping does the has_ part a bit differently because it
wants to carry that flag over to the rest of the code. See 3cba22c9ad
("migration: Fix block_bitmap_mapping migration"). In that case, even if
the list is empty, it needs to know when it was made empty on purpose to
carry on with the removal of the mappings. In your case, it doesn't
matter, empty is empty, whether forcefully, or because it was never
set. Right?

(this migrate_params code is tricky, FYI I'm reworking that in:
https://lore.kernel.org/r/20250603013810.4772-1-farosas@suse.de)

>> 
>>> +        dest->fds = params->fds;
>>> +    }
>>>   }
>>>   
>>>   static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>>> @@ -1429,6 +1452,13 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>>>       if (params->has_direct_io) {
>>>           s->parameters.direct_io = params->direct_io;
>>>       }
>>> +
>>> +    if (params->has_fds) {
>>> +        qapi_free_FdsTargetList(s->parameters.fds);
>>> +
>>> +        s->parameters.has_fds = true;
>>> +        s->parameters.fds = QAPI_CLONE(FdsTargetList, params->fds);
>> 
>> Same here, has_fds should always be true in s->parameters.
>> 
>>> +    }
>>>   }
>>>   
>>>   void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>>> diff --git a/migration/options.h b/migration/options.h
>>> index 82d839709e..a79472a235 100644
>>> --- a/migration/options.h
>>> +++ b/migration/options.h
>>> @@ -87,6 +87,8 @@ const char *migrate_tls_hostname(void);
>>>   uint64_t migrate_xbzrle_cache_size(void);
>>>   ZeroPageDetection migrate_zero_page_detection(void);
>>>   
>>> +bool migrate_fds_virtio_net(void);
>>> +
>>>   /* parameters helpers */
>>>   
>>>   bool migrate_params_check(MigrationParameters *params, Error **errp);
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 2387c21e9c..6ef9629c6d 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -747,6 +747,19 @@
>>>         '*transform': 'BitmapMigrationBitmapAliasTransform'
>>>     } }
>>>   
>>> +##
>>> +# @FdsTarget:
>>> +#
>>> +# @virtio-net: Enable live backend migration for virtio-net.
>> 
>> So you're assuming normal migration is "non-live backend migration"
>> because the backend is stopped and started again on the destination. I
>> think it makes sense.
>> 
>>> +#     The only supported backend is TAP device. When enabled, TAP fds
>>> +#     and all related state is passed to target QEMU through migration
>>> +#     channel (which should be unix socket).
>>> +#
>>> +# Since: 10.2
>>> +##
>>> +{ 'enum': 'FdsTarget',
>>> +  'data': [ 'virtio-net' ] }
>>> +
>>>   ##
>>>   # @BitmapMigrationNodeAlias:
>>>   #
>>> @@ -924,10 +937,14 @@
>>>   #     only has effect if the @mapped-ram capability is enabled.
>>>   #     (Since 9.1)
>>>   #
>>> +# @fds: List of targets to enable live-backend migration for. This
>>> +#     requires migration channel to be a unix socket (to pass fds
>>> +#     through). (Since 10.2)
>> 
>> I think I prefer live-backend as written here rather than the non
>> hyphenated version above. This clears up the ambiguity and can be used
>> in variable names, as a name for the feature and ... _thinks really
>> hard_ ... as the parameter name! (maybe)
>> 
>> On that note, fds is just too broad, I'm sure you know that, but to be
>> specific, we already have fd migration, multifd, fdset (as argument of
>> file: migration), etc. Talking just about the user interface here, of
>> course.
>> 
>> Also: "@fds: List of targets..." is super confusing.
>> 
>> And although "to pass fds through" is helping clarify the meaning of
>> @fds, it exposes implementation details on the QAPI, I don't think it's
>> relevant in the parameter description.
>
> Agree. I see all this mess, each time I come with some new name:
> live-tap, live-backend, fds-passing, fds migration, local tap migration, fds...
> Finally, only one should be chosen for all names and in documentation.
>

Yeah, not to mention the similarities with CPR. I thought of borrowing
the "transfer" term to underline that's a common concept between the two
approaches, but I'm not sure it's worth the trouble.

[live-]backend-transfer

> With your comments, "live-backend" really looks the best one.
>
> Still, we can't say live-backends: [virtio-net], as virtio-net is not
> a backed.
>
> Maybe
>     
>     live-backends: [virtio-net-tap]
>
> to show that it's about virtio-net+TAP pair.
>

Works for me.

>> 
>>> +#
>>>   # Features:
>>>   #
>>> -# @unstable: Members @x-checkpoint-delay and
>>> -#     @x-vcpu-dirty-limit-period are experimental.
>
>
> Thanks!