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.
>
> 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
