On 25/02/2025 22:48, Peter Xu wrote:
> On Tue, Feb 25, 2025 at 06:37:21AM +0000, Zhijian Li (Fujitsu) wrote:
>>
>>
>> On 25/02/2025 03:58, Peter Xu wrote:
>>> On Fri, Feb 21, 2025 at 02:36:09PM +0800, Li Zhijian wrote:
>>>> Similar to migration_channels_and_transport_compatible(), introduce a
>>>> new helper migration_capabilities_and_transport_compatible() to check if
>>>> the capabilites is compatible with the transport.
>>>>
>>>> Currently, only move the capabilities vs RDMA transport to this
>>>> function.
>>>>
>>>> Signed-off-by: Li Zhijian <lizhij...@fujitsu.com>
>>>
>>> Yeah this is even better, thanks.
>>>
>>> Reviewed-by: Peter Xu <pet...@redhat.com>
>>
>> Hi Peter,
>>
>> Thinking one step further, this patch looks promising and can check
>> most situations. However, on the destination side, if the user first
>> specifies '-incoming' (with the startup parameter -incoming xxx or
>> migrate_incoming xxx) and then 'migrate_set_capability xxx on',
>> the function migration_capabilities_and_transport_compatible() will
>> not be called to check compatibility, which might lead to migration failure.
>>
>> To address this, without introducing a new member 'transport' into the
>> MigrationState
>> structure, the code might need to be adjusted to this:
>>
>> The question is whether we need to consider it now(in this patch set)?
>
> We can do that in one patch.
Okay, please ignore the V3 and take another look at the V4 which integrated your
below suggestion.
Thanks
>
>>
>> static bool migration_transport_compatible(MigrationAddress *addr, Error
>> **errp)
>> {
>> return migration_channels_and_transport_compatible(addr, errp) &&
>> - migration_capabilities_and_transport_compatible(addr, errp);
>> + migration_capabilities_and_transport_compatible(addr->transport,
>> + migrate_get_current()->capabilities, errp);
>
> Here IMHO we could make migration_capabilities_and_transport_compatible()
> taking addr+errp like before, then..
>
>> }
>>
>> static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
>> diff --git a/migration/options.c b/migration/options.c
>> index bb259d192a9..59f0ed5b528 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -439,6 +439,29 @@ static bool migrate_incoming_started(void)
>> return !!migration_incoming_get_current()->transport_data;
>> }
>>
>> +bool
>> +migration_capabilities_and_transport_compatible(MigrationAddressType
>> transport,
>> + bool *new_caps,
>> + Error **errp)
>> +{
>
> .. here fetch global capability list and feed it.
>
>> + if (transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>
> [1]
>
>> + if (new_caps[MIGRATION_CAPABILITY_XBZRLE]) {
>> + error_setg(errp, "RDMA and XBZRLE can't be used together");
>> + return false;
>> + }
>> + if (new_caps[MIGRATION_CAPABILITY_MULTIFD]) {
>> + error_setg(errp, "RDMA and multifd can't be used together");
>> + return false;
>> + }
>> + if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
>> + error_setg(errp, "RDMA and postcopy-ram can't be used
>> together");
>> + return false;
>> + }
>
> We could introduce migration_rdma_caps_check(&caps, errp) for this chunk
> (since [1]), then...
>
>> + }
>> +
>> + return true;
>> +}
>> +
>> /**
>> * @migration_caps_check - check capability compatibility
>> *
>> @@ -602,6 +625,15 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
>> Error **errp)
>> }
>> }
>>
>> + /*
>> + * In destination side, check the cases that capability is being set
>> + * after incoming thread has started.
>> + */
>> + if (migrate_rdma() &&
>> + !migration_capabilities_and_transport_compatible(
>> + MIGRATION_ADDRESS_TYPE_RDMA, new_caps, errp)) {
>> + return false;
>> + }
>
> ... use migration_rdma_caps_check() here, might be slightly more readable:
>
> if (migrate_rdma() && !migration_rdma_caps_check(new_caps, errp)) {
> return false;
> }
>
>> return true;
>> }
>>
>> diff --git a/migration/options.h b/migration/options.h
>> index 762be4e641a..ca6a40e7545 100644
>> --- a/migration/options.h
>> +++ b/migration/options.h
>> @@ -58,6 +58,9 @@ bool migrate_tls(void);
>> /* capabilities helpers */
>>
>> bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
>> +bool
>> +migration_capabilities_and_transport_compatible(MigrationAddressType
>> transport,
>> + bool *new_caps, Error
>> **errp);
>>
>>>
>
