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 <lizhijian@fujitsu.com>
---
migration/migration.c | 36 ++++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index c597aa707e5..2eacae25e0e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -238,6 +238,30 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
return true;
}
+static bool
+migration_capabilities_and_transport_compatible(MigrationAddress *addr,
+ Error **errp)
+{
+ if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+ if (migrate_xbzrle()) {
+ error_setg(errp, "RDMA and XBZRLE can't be used together");
+ return false;
+ }
+ if (migrate_multifd()) {
+ error_setg(errp, "RDMA and multifd can't be used together");
+ return false;
+ }
+ }
+
+ return true;
+}
+
+static bool migration_transport_compatible(MigrationAddress *addr, Error **errp)
+{
+ return migration_channels_and_transport_compatible(addr, errp) &&
+ migration_capabilities_and_transport_compatible(addr, errp);
+}
+
static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
{
uintptr_t a = (uintptr_t) ap, b = (uintptr_t) bp;
@@ -716,7 +740,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
}
/* transport mechanism not suitable for migration? */
- if (!migration_channels_and_transport_compatible(addr, errp)) {
+ if (!migration_transport_compatible(addr, errp)) {
return;
}
@@ -735,14 +759,6 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
}
#ifdef CONFIG_RDMA
} else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
- if (migrate_xbzrle()) {
- error_setg(errp, "RDMA and XBZRLE can't be used together");
- return;
- }
- if (migrate_multifd()) {
- error_setg(errp, "RDMA and multifd can't be used together");
- return;
- }
rdma_start_incoming_migration(&addr->u.rdma, errp);
#endif
} else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
@@ -2159,7 +2175,7 @@ void qmp_migrate(const char *uri, bool has_channels,
}
/* transport mechanism not suitable for migration? */
- if (!migration_channels_and_transport_compatible(addr, errp)) {
+ if (!migration_transport_compatible(addr, errp)) {
return;
}
--
2.44.0On 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 <lizhijian@fujitsu.com> Yeah this is even better, thanks. Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu
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 <lizhijian@fujitsu.com>
>
> Yeah this is even better, thanks.
>
> Reviewed-by: Peter Xu <peterx@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)?
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);
}
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)
+{
+ if (transport == MIGRATION_ADDRESS_TYPE_RDMA) {
+ 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;
+ }
+ }
+
+ 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;
+ }
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);
>
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 <lizhijian@fujitsu.com>
> >
> > Yeah this is even better, thanks.
> >
> > Reviewed-by: Peter Xu <peterx@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.
>
> 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);
>
> >
--
Peter Xu
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 <lizhijian@fujitsu.com>
>>>
>>> Yeah this is even better, thanks.
>>>
>>> Reviewed-by: Peter Xu <peterx@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);
>>
>>>
>
© 2016 - 2026 Red Hat, Inc.